Add graph walking functions to Graph and GraphOperation #232

rnett · 2021-03-04T23:38:16Z

Extracted from #211, adds graph walking and a minimal subgraph function.

Also fixes a bug in Constant where the scope wasn't applied (including device).

rnett · 2021-03-05T00:24:37Z

I may need to add a flag to include variables, too.

rnett · 2021-03-05T00:48:29Z

The CI seems to be broken again, I'm not having issues running install for the whole project locally, but there's javadoc resolution errors from framework.

Craigacp

There are a lot of unnecessary formatting changes in this PR. Could you back those out?

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/Graph.java

Craigacp · 2021-03-05T03:08:37Z

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/Graph.java

+ }
+
+ if (op.numControlInputs() + op.numInputs() == 0) {
+ if ((forbiddenBodyOps != null && forbiddenBodyOps.contains(op.type()))


Initialising these to Collections.emptySet() internally even if we don't make users do it will make this if a lot less ugly.

See one of the other comments, nulls are necessary to distinguish from "only allow the empty set" which would error whenever something is in the body. Optionally we could do that behavior (i.e. allow everything) for the empty set, but imo nulls is clearer (and what if that's what you want?).

Why would we want the behaviour of only allowing the empty set? So you could catch the runtime exception and have different behaviour if there were intermediate nodes?

Anything that implies users have to catch a subclass of RuntimeException isn't good.

It's for 0 input intermediate nodes specifically, i.e. things that don't depend on the inputs. Generally Constant is ok and the others (Placeholder, Variable, etc) are ok, but since I'm making it a parameter "none" is an option.

Ok. I'm still not sure that the semantics of this is quite right, but given we've not stabilised the API yet lets add it in and use it a bit first before making further decisions.

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/GraphOperation.java

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/op/core/Constant.java

Craigacp · 2021-03-05T03:37:05Z

It might be useful to have two extra graph walking functions one which traces all the ops which depend on a set of inputs, and one which traces all the ops which are depended on by a set of outputs. That way you can take any graph and figure out what you need to feed into it for a given output. That's not likely to be in a production system, but it would be super useful when poking a model in jshell. I've had to write ad-hoc things which do that when I was trying to understand the TF 1.x graph structure for saving, and having a supported method would be nice.

rnett · 2021-03-05T04:54:23Z

It might be useful to have two extra graph walking functions one which traces all the ops which depend on a set of inputs, and one which traces all the ops which are depended on by a set of outputs. That way you can take any graph and figure out what you need to feed into it for a given output. That's not likely to be in a production system, but it would be super useful when poking a model in jshell. I've had to write ad-hoc things which do that when I was trying to understand the TF 1.x graph structure for saving, and having a supported method would be nice.

Yeah, I can do that.

rnett · 2021-03-05T05:07:05Z

There are a lot of unnecessary formatting changes in this PR. Could you back those out?

This is due to my auto format on commit, so not really, at least not without it being a big pain. It's google-java-format, though, so it should really have already been done. Imo we should add a format check for PRs in the CI (and format everything existing to match), at least once it's not having time issues.

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/Graph.java

Craigacp

LGTM.

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/Graph.java

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/GraphOperation.java

karllessard · 2021-03-17T14:15:00Z

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/Graph.java

+ * @see #completeSubgraph(Set, Set, boolean)
+ */
+ public synchronized Set<GraphOperation> completeSubgraph(Set<Operand<?>> inputs, Set<Operand<?>> outputs,
+ Set<String> allowedNoInputBodyOps, Set<String> forbiddenNoInputBodyOps) {


These two last extra parameters obfuscate a bit the overall solution, which would be very simple without them. Why can't we simply include all ops we encounter without this logic, whether they have inputs available or not, and let the caller filter them out after if he needs to?

Yeah, that's fine. I don't like how easy it is to end up including things upstream from the inputs in the body then, but I don't see much of a way around it, since we can't just forbid it altogether.

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

karllessard · 2021-03-18T01:28:44Z

@rnett looks good, are you done with the changes? If so, can you please check why the quick build has failed?

rnett · 2021-03-18T02:10:16Z

It's the javadoc issues in framework. I think what's happening is core-api is set to ignore Javadoc failures, so Javadoc fails but the build succeeds, but then framework, which isn't set to ignore Javadoc failures, fails when it tries to depend on them. I'm pretty sure this is an issue on master as well, I didn't add anything that should fail.

See core failure (that is ignored), framework failure, and the core pom.

It's causing all recent PRs to fail the CI.

karllessard · 2021-03-18T14:28:40Z

Ok, I see. Indeed, the javadoc validation is disabled in the tensorflow-core-api since this generated documentation from Markdown is not 100% legit right now.

Now what you are saying is that the framework documentation with links to the generated classes fail to pass this validation because it tries to validate the documentation it links to?

I was not aware of that but I guess it's possible, @JimClarke5 can we clean up the framework documentation to at least fix the CI build, even if it means to disable these links in it?

I will merge this PR then, thanks @rnett !

JimClarke5 · 2021-03-18T15:06:03Z

There is a C library for commonmark, but it would need to be extended with a JavaDoc renderer based on the HtmlRenderer. I have done this with Python and am now working on the Java version. I could look at the C version and determine what needs to be done.

karllessard · 2021-03-18T17:31:27Z

Ok @JimClarke5 I'll let you judge how long it would take but for now the priority is really to fix the CI build so that snapshots get refreshed on Sonatype after merging a PR. Do you think we can simply fix the framework quickly by removing links to the generated ops?

JimClarke5 · 2021-03-18T17:38:55Z

@karllessard Let me experiment with the CommonMark-cmark library. That would be the quickest fix and incorporate it into SourceWriter.cc

rnett · 2021-03-18T18:22:56Z

Couldn't we, for now, just let framework build w/ failing javadocs? W/ a note to fix before the next release or something.

JimClarke5 · 2021-03-18T18:29:11Z

@karllessard please explain exactly what you mean by “ quickly by removing links to the generated ops?”

JimClarke5 · 2021-03-18T21:40:52Z

@karllessard As a test, I have been able to transform the markup from api_def_UnsortedSegmentMax.pbtxt

  description: <<END
Read
[the section on segmentation](https://tensorflow.org/api_docs/python/tf/math#Segmentation)
for an explanation of segments.

This operator is similar to the unsorted segment sum operator found
[(here)](../../../api_docs/python/math_ops.md#UnsortedSegmentSum).
Instead of computing the sum over segments, it computes the maximum such that:

\\(output_i = \max_{j...} data[j...]\\) where max is over tuples `j...` such
that `segment_ids[j...] == i`.

If the maximum is empty for a given segment ID `i`, it outputs the smallest
possible value for the specific numeric type,
`output[i] = numeric_limits<T>::lowest()`.

If the given segment ID `i` is negative, then the corresponding value is
dropped, and will not be included in the result.

to

/**
 * Read <a href="https://tensorflow.org/api_docs/python/tf/math#Segmentation">the section on segmentation</a> for an  
 * explanation of segments.This operator is similar to the unsorted segment sum operator found        
 * <a href="../../../api_docs/python/math_ops.md#UnsortedSegmentSum">(here)</a>.Instead of computing the sum over
 *  segments, it computes the maximum such that:\(output_i = \max_{j...} data[j...]\) where max is over tuples {@code j... } 
 * such that {@code segment_ids[j...] == i }.If the maximum is empty for a given segment ID {@code i }, it outputs the 
 * smallestpossible value for the specific numeric type,{@code output[i] = numeric_limits<T>::lowest() }.If the given segment 
 * ID {@code i } is negative, then the corresponding value isdropped, and will not be included in the result.
 * /

The only issue so far is the relative URL ../../../api_docs/python/math_ops.md#UnsortedSegmentSum in the second link.
With a little brute force, I think I can change that to {@link MathOps#unsupportedSegmentSum}.

I can change SourceWriter.cc to call out to new C methods to handle the JavaDoc rendering and in turn link in the cmark C library. I will also have to add the external cmark source to bazel_bin-external ( I think ). Is external pulled down from GitHub somewhere? Also, there may be some edge cases I have to handle as I expand out to the full api_def directory.

karllessard · 2021-03-20T01:24:23Z

@JimClarke5 , I would say don't put too much effort of this C++ markdown converter because @rnett is working on porting the op generator to Java anyway (#244). Unless Ryan you think that we could use it to convert the documentation as we generate the new ops.pb file?

rnett · 2021-03-20T03:45:10Z

I mean, we probably could, but given this is a Java library originally (if I understood things right) it's probably easier to wait.

Imo skipping the Javadoc generation on the CI until that's done works fine as an "emergency solution".

rnett mentioned this pull request Mar 5, 2021

Native functions v2 #233

Merged

Craigacp reviewed Mar 5, 2021

View reviewed changes

rnett requested a review from Craigacp March 9, 2021 03:50

Craigacp reviewed Mar 9, 2021

View reviewed changes

Craigacp previously approved these changes Mar 9, 2021

View reviewed changes

karllessard reviewed Mar 17, 2021

View reviewed changes

rnett dismissed Craigacp’s stale review via 7f5fcf1 March 17, 2021 20:37

rnett added 8 commits March 17, 2021 13:45

Add graph walking functions to Graph and GraphOperation

2c7fa1b

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

Add subgraph option to allow constants in body

b3306d5

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

Generalize allowing 0-input ops in the body

768655f

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

Fixes, upstream and downstream functions

8702dd3

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

Small fixes

09d30b5

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

Back to ArrayDeque

8a8440c

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

Small fixes

f5c52dd

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

Change completeSubgraph signature, leave filtering to users

ff37a27

Signed-off-by: Ryan Nett <rnett@calpoly.edu>

rnett force-pushed the rn_graph_walking branch from f308694 to ff37a27 Compare March 17, 2021 20:45

karllessard approved these changes Mar 18, 2021

View reviewed changes

karllessard merged commit 5ca958d into tensorflow:master Mar 18, 2021

JimClarke5 mentioned this pull request Apr 29, 2021

Need a way to pass trainable Variables to Optimizer #307

Open

rnett deleted the rn_graph_walking branch October 17, 2021 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add graph walking functions to Graph and GraphOperation #232

Add graph walking functions to Graph and GraphOperation #232

rnett commented Mar 4, 2021 •

edited

Loading

rnett commented Mar 5, 2021

rnett commented Mar 5, 2021

Craigacp left a comment

Craigacp Mar 5, 2021

rnett Mar 5, 2021

Craigacp Mar 5, 2021

rnett Mar 7, 2021

Craigacp Mar 9, 2021

Craigacp commented Mar 5, 2021

rnett commented Mar 5, 2021

rnett commented Mar 5, 2021

Craigacp left a comment

karllessard Mar 17, 2021

rnett Mar 17, 2021

karllessard commented Mar 18, 2021

rnett commented Mar 18, 2021 •

edited

Loading

karllessard commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

karllessard commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

rnett commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

karllessard commented Mar 20, 2021

rnett commented Mar 20, 2021 •

edited

Loading

Add graph walking functions to Graph and GraphOperation #232

Add graph walking functions to Graph and GraphOperation #232

Conversation

rnett commented Mar 4, 2021 • edited Loading

rnett commented Mar 5, 2021

rnett commented Mar 5, 2021

Craigacp left a comment

Choose a reason for hiding this comment

Craigacp Mar 5, 2021

Choose a reason for hiding this comment

rnett Mar 5, 2021

Choose a reason for hiding this comment

Craigacp Mar 5, 2021

Choose a reason for hiding this comment

rnett Mar 7, 2021

Choose a reason for hiding this comment

Craigacp Mar 9, 2021

Choose a reason for hiding this comment

Craigacp commented Mar 5, 2021

rnett commented Mar 5, 2021

rnett commented Mar 5, 2021

Craigacp left a comment

Choose a reason for hiding this comment

karllessard Mar 17, 2021

Choose a reason for hiding this comment

rnett Mar 17, 2021

Choose a reason for hiding this comment

karllessard commented Mar 18, 2021

rnett commented Mar 18, 2021 • edited Loading

karllessard commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

karllessard commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

rnett commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

JimClarke5 commented Mar 18, 2021

karllessard commented Mar 20, 2021

rnett commented Mar 20, 2021 • edited Loading

rnett commented Mar 4, 2021 •

edited

Loading

rnett commented Mar 18, 2021 •

edited

Loading

rnett commented Mar 20, 2021 •

edited

Loading