Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,12 @@ jobs:
mvn clean verify -P '!showcase,enable-integration-tests,loggingTestBase,disabledLogging' \
--batch-mode \
--no-transfer-progress
- name: Showcase integration tests - Protobuf 3 compatibility with third party library Tensorflow
working-directory: java-showcase
run: |
mvn clean verify -P 'enable-integration-tests,protobuf3,showcase' \
--batch-mode \
--no-transfer-progress

showcase-clirr:
if: ${{ github.base_ref != '' }} # Only execute on pull_request trigger event
Expand Down
27 changes: 27 additions & 0 deletions java-showcase/gapic-showcase/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,14 @@
<scope>test</scope>
</dependency>

<!-- Protobuf 3.x compatibility tests, tensorflow depends on protobuf 3.x gen code and runtime -->
<dependency>
<groupId>org.tensorflow</groupId>
<artifactId>tensorflow-core-platform</artifactId>
<version>1.1.0</version>
<scope>test</scope>
</dependency>

<!-- Otel testing libraries -->
<dependency>
<groupId>io.opentelemetry</groupId>
Expand Down Expand Up @@ -361,6 +369,25 @@
</plugins>
</build>
</profile>
<profile>
<id>protobuf3</id>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<testExcludes>
<testExclude>**/com/google/showcase/v1beta1/it/logging/*.java</testExclude>
</testExcludes>
<testIncludes>
<testInclude>**/com/google/showcase/v1beta1/it/ITProtobuf3Compatibility.java</testInclude>
</testIncludes>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>

</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package com.google.showcase.v1beta1.it;

import static com.google.common.truth.Truth.assertThat;

import org.junit.jupiter.api.Test;
import org.tensorflow.Graph;
import org.tensorflow.Session;
import org.tensorflow.op.Ops;
import org.tensorflow.op.core.Constant;
import org.tensorflow.op.math.Add;
import org.tensorflow.proto.GraphDef;
import org.tensorflow.types.TInt32;

/**
* Tensorflow depends on protobuf 3.x gen code and runtime, we test it in showcase module to prove
* that it works with protobuf 4.33+ gen code and runtime that comes with client libraries.
*/
class ITProtobuf3Compatibility {

@Test
void testTensorflow_helloWorldExample() {
try (Graph graph = new Graph()) {
// Hello world example for "10 + 32" operation.
Ops tf = Ops.create(graph);

int expectedValue1 = 10;
int expectedValue2 = 32;
int expectedSum = 42;

String name1 = "constant1";
String name2 = "constant2";

Constant<TInt32> constant1 = tf.withName(name1).constant(expectedValue1);
Constant<TInt32> constant2 = tf.withName(name2).constant(expectedValue2);

Add<TInt32> sum = tf.math.add(constant1, constant2);

try (Session s = new Session(graph)) {
try (TInt32 result = (TInt32) s.runner().fetch(sum).run().get(0)) {
int actualResult = result.getInt();
assertThat(actualResult).isEqualTo(expectedSum);
}
}

// GraphDef is a protobuf gen code.
GraphDef graphDef = graph.toGraphDef();

// Inspect the protobuf gen code
Integer actual1 = getValueFromGraphDefByName(graphDef, name1);
Integer actual2 = getValueFromGraphDefByName(graphDef, name2);

assertThat(actual1).isEqualTo(expectedValue1);
assertThat(actual2).isEqualTo(expectedValue2);
}
}

private static Integer getValueFromGraphDefByName(GraphDef graphDef, String name1) {
return graphDef.getNodeList().stream()
.filter(nodeDef -> nodeDef.getName().equals(name1))
.findFirst()
.get()
.getAttrOrThrow("value")
.getTensor()
.getIntValList()
.get(0);
}
Comment on lines +57 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation of getValueFromGraphDefByName uses .get() on an Optional and .get(0) on a List without checking for presence or emptiness. This can lead to NoSuchElementException or IndexOutOfBoundsException if the expected node or value is not found, which can make debugging test failures difficult. Using orElseThrow with a descriptive message would provide more informative error messages.

Additionally, the parameter name name1 is a bit specific for a general-purpose helper method. Renaming it to name would improve clarity.

I've suggested a change that addresses both points, making the method more robust and readable.

  private static Integer getValueFromGraphDefByName(GraphDef graphDef, String name) {
    return graphDef.getNodeList().stream()
        .filter(nodeDef -> nodeDef.getName().equals(name))
        .findFirst()
        .orElseThrow(() -> new AssertionError("Node not found: " + name))
        .getAttrOrThrow("value")
        .getTensor()
        .getIntValList()
        .stream()
        .findFirst()
        .orElseThrow(() -> new AssertionError("Node has no int value: " + name));
  }

}
2 changes: 2 additions & 0 deletions java-showcase/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
<!-- Do not compile by default (without logging deps) -->
<testExcludes>
<testExclude>**/com/google/showcase/v1beta1/it/logging/*.java</testExclude>
<testExclude>**/com/google/showcase/v1beta1/it/ITProtobuf3Compatibility.java</testExclude>
</testExcludes>
</configuration>
</plugin>
Expand Down Expand Up @@ -120,6 +121,7 @@
<!-- Do not compile by default (without logging deps) -->
<testExcludes>
<testExclude>**/com/google/showcase/v1beta1/it/logging/*.java</testExclude>
<testExclude>**/com/google/showcase/v1beta1/it/ITProtobuf3Compatibility.java</testExclude>
</testExcludes>
</configuration>
</plugin>
Expand Down
Loading