Add option to run TensorFlow job on the preferred device (via Scope) #159
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karllessard
requested changes
Nov 28, 2020
tensorflow-core/tensorflow-core-api/src/gen/annotations/org/tensorflow/op/Ops.java
Outdated
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/gen/java/org/tensorflow/op/math/Abs.java
Outdated
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/op/Scope.java
Outdated
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/test/java/org/tensorflow/op/core/ConstantTest.java
Outdated
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/test/java/org/tensorflow/op/core/ConstantTest.java
Outdated
Show resolved
Hide resolved
...orflow-core-generator/src/main/java/org/tensorflow/processor/operator/OperatorProcessor.java
Show resolved
Hide resolved
|
Thank you, it was a draft PR with code changes, but without code
generation, I will update the PR and notify you
вс, 29 нояб. 2020 г., 1:13 Karl Lessard <[email protected]>:
… ***@***.**** requested changes on this pull request.
------------------------------
In
tensorflow-core/tensorflow-core-api/src/gen/annotations/org/tensorflow/op/Ops.java
<#159 (comment)>:
> @@ -19,11 +19,8 @@
import java.nio.charset.Charset;
import java.util.List;
-import org.tensorflow.DataType;
-import org.tensorflow.EagerSession;
-import org.tensorflow.ExecutionEnvironment;
-import org.tensorflow.Operand;
-import org.tensorflow.Tensor;
+
+import org.tensorflow.*;
Let's avoid wildcard imports, as per Google Java style guide
<https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports>
suggestion.
------------------------------
In
tensorflow-core/tensorflow-core-api/src/gen/java/org/tensorflow/op/math/Abs.java
<#159 (comment)>:
> @@ -50,6 +50,7 @@
@endpoint(describeByClass = true)
public static <T extends TNumber> Abs<T> create(Scope scope, Operand<T> x) {
OperationBuilder opBuilder = scope.env().opBuilder("Abs", scope.makeOpName("Abs"));
+ opBuilder.setDevice(scope.makeDeviceString());
All generated ops should have been update by your change in the op
generator above, they should be all updated by this PR as well, not only
this one (there must be 1000+ files to be added).
------------------------------
In
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/op/Scope.java
<#159 (comment)>:
> @@ -121,7 +123,12 @@ public Scope withSubScope(String childScopeName) {
* @throws IllegalArgumentException if the name is invalid
*/
public Scope withName(String opName) {
- return new Scope(env, nameScope.withName(opName), controlDependencies);
+ return new Scope(env, nameScope.withName(opName), controlDependencies, deviceSpec);
+ }
+
+ // TODO: add docs
TODO: add docs :)
------------------------------
In
tensorflow-core/tensorflow-core-api/src/test/java/org/tensorflow/op/core/ConstantTest.java
<#159 (comment)>:
> import org.junit.jupiter.api.Test;
-import org.tensorflow.AutoCloseableList;
-import org.tensorflow.Graph;
-import org.tensorflow.Session;
-import org.tensorflow.Tensor;
+import org.tensorflow.*;
No wildcard imports
------------------------------
In
tensorflow-core/tensorflow-core-api/src/test/java/org/tensorflow/op/core/ConstantTest.java
<#159 (comment)>:
> +
+ Output<TInt32> aOps = g
+ .opBuilder("Const", "aOps")
+ .setAttr("dtype", a.dataType())
+ .setAttr("value", a)
+ .setDevice("/job:localhost/replica:0/task:0/device:CPU:0")
+ .build()
+ .output(0);
+ DeviceSpec deviceSpec = DeviceSpec.newBuilder()
+ .job("localhost")
+ .replica(0)
+ .task(1)
+ .deviceType(DeviceSpec.DeviceType.CPU)
+ .build();
+
+ //DeviceSpec deviceSpec = DeviceSpec.newBuilder().build();
This comment is probably a leftover
------------------------------
In
tensorflow-core/tensorflow-core-generator/src/main/java/org/tensorflow/processor/operator/OperatorProcessor.java
<#159 (comment)>:
> @@ -537,6 +538,18 @@ private static TypeSpec buildTopClass(OpsSpec spec) {
T_SCOPE)
.build());
+ opsBuilder.addMethod(
+ MethodSpec.methodBuilder("withDevice")
+ .addModifiers(Modifier.PUBLIC)
+ .addParameter(T_DEVICE_SPEC, "deviceSpec")
+ .returns(T_OPS)
+ .addStatement("return new Ops(scope.withDevice(deviceSpec))")
+ .addJavadoc(
+ "Returns an API that uses the provided DeviceSpec for an op.\n\n"
+ + ***@***.*** ***@***.*** $T#withDevice(DeviceSpec)}\n",
+ T_SCOPE)
+ .build());
+
The Ops class should now have this new method in it but it does not
appear in the version you've committed.
Also, I would rephrase its documentation for something like this:
"Returns an API that places the created operations on the device(s)
matching the provided spec. @see <https://github.com/see>..."
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#159 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJEUHLO5NBCOROTOWXJVXTSSFYW3ANCNFSM4UE5ICSQ>
.
|
|
Ah, did you marked it as a draft when you've created your PR? My GitHub iPad app did not tell me this, maybe a missing feature in the app. But yes, I think with only these small changes, your PR will be ready to be merged, thanks! |
|
No, I didn't mark, sorry for that, but your comments are useful, because
now it's clear what should be checked for successful PR
вс, 29 нояб. 2020 г., 17:35 Karl Lessard <[email protected]>:
… Ah, did you marked it as a draft when you've created your PR? My GitHub
iPad app did not tell me this, maybe a missing feature in the app. But yes,
I think with only these small changes, your PR will be ready to be merged,
thanks!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#159 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJEUHNFSXENZ6SKG5DIDVLSSJL2RANCNFSM4UE5ICSQ>
.
|
|
@karllessard I've updated the PR and fixed all your proposals, please have a look |
karllessard
reviewed
Nov 30, 2020
| // Add Device String | ||
| writer->Append("opBuilder.setDevice(scope.makeDeviceString());"); | ||
| writer->EndLine(); | ||
|
|
There was a problem hiding this comment.
Hey @zaleslaw , I have another change to propose you that I just thought of. I think if we merge together scope.applyControlDependencies and this new step for adding the device string to the opBuilder would make the whole process of building an op simpler, especially with non-generated ones.
So what about that:
- We rename
scope.applyControlDependencies(opBuilder)toscope.apply(opBuilder) - In
Scope.apply, we apply both control dependencies, the device string (and eventually more data if needed)
By doing so, we also won't need scope.makeDeviceString() anymore. Let me know what you think
|
Need a time to think about it
пн, 30 нояб. 2020 г. в 17:19, Karl Lessard <[email protected]>:
… ***@***.**** commented on this pull request.
------------------------------
In
tensorflow-core/tensorflow-core-api/src/bazel/op_generator/op_generator.cc
<#159 (comment)>:
> @@ -246,6 +246,10 @@ void RenderFactoryMethods(const OpSpec& op, const Type& op_class,
writer->EndLine();
}
}
+ // Add Device String
+ writer->Append("opBuilder.setDevice(scope.makeDeviceString());");
+ writer->EndLine();
+
Hey @zaleslaw <https://github.com/zaleslaw> , I have another change to
propose you that I just thought of. I think if we merge together
scope.applyControlDependencies and this new step for adding the device
string to the opBuilder would make the whole process of building an op
simpler, especially with non-generated ones.
So what about that:
1. We rename scope.applyControlDependencies(opBuilder) to
scope.apply(opBuilder)
2. In Scope.apply, we apply both control dependencies, the device
string (and eventually more data if needed)
By doing so, we also won't need scope.makeDeviceString() anymore. Let me
know what you think
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#159 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJEUHNB3AY7GGXNB2HE65DSSOSVRANCNFSM4UE5ICSQ>
.
|
|
@karllessard I've updated a PR according your refactoring idea, but I have some troubles with quick build Could you pleas help with this issue? |
|
@zaleslaw since your PR changes the generated files, we shouldn't use quick build anyway, I'll force a full build and see how it goes (independently to this, I suspect the missing artifact is due again to our trouble building all native artifacts on GitHub actions...) |
karllessard
approved these changes
Dec 3, 2020
|
Full build passed (well partially, GPU-MKL timed out again on Linux and it's unrelated), thanks @zaleslaw ! |
|
Happy to hear! |
|
Thanks @zaleslaw and @karllessard!! |
rnett
pushed a commit
to rnett/tensorflow-java
that referenced
this pull request
Dec 28, 2020
…ensorflow#159) * Draft for DeviceSpec integration * Added generation for Ops * Removed something from generated Ops * Finalized the PR * Refactored Scope.apply method * Handle case of separate usage of control dependencies in Init method
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to the issue #77