-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing callbacks #57
Conversation
For the first test
For the second test
|
As we can see from the CGs, the functions being called are not showing up in the CG. Only the APIs that are being used. |
I ran an example on the TF Estimator referenced in the issue (wala#92 (comment)), and the CG is similar to what we see here. |
...bm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestTensorflowModel.java
Outdated
Show resolved
Hide resolved
The CG for the first test:
|
The CG for the second test:
|
BTW, these are examples of ones that are missing. Are there more that we need that are obvious? |
...bm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestTensorflowModel.java
Outdated
Show resolved
Hide resolved
...bm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestTensorflowModel.java
Outdated
Show resolved
Hide resolved
Actually it is MirrorStrategy or TPUStrategy. This is where they are initializing |
Could we have the diffs of these? |
Here's the diff of your CGs after some editing: 29a30,33
> - invokevirtual < PythonLoader, LRoot, do()LRoot; >@3
> -> Node: <Code body of function Lscript tf2_test_callbacks2.py/replica_fn> Context: CallStringContext: [ tensorflow.distribute.run.run.do()LRoot;@3 ]
>
> Node: <Code body of function Lscript tf2_test_callbacks2.py/replica_fn> Context: CallStringContext: [ tensorflow.distribute.run.run.do()LRoot;@3 ] This is certainly promising, but the tensor analysis still isn't picking up the tensor parameters. My guess is that the argument (in the second test; I'm not looking at the first) isn't being connected to the parameter in the XML. |
It would also be helpful to see the IR and pointer analyses. |
It may also be helpful to have a function that isn't a call back in the example and that directly takes a tensor parameter. I will also say that if you simply return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tensor parameters don't look right.
...bm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestTensorflowModel.java
Outdated
Show resolved
Hide resolved
...bm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestTensorflowModel.java
Outdated
Show resolved
Hide resolved
|
||
<package name="tensorflow/distribute/run"> | ||
<class name="run" allocatable="true"> | ||
<method name="do" descriptor="()LRoot;" numArgs="2" paramNames="self fn"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at https://www.tensorflow.org/api_docs/python/tf/distribute/Strategy#run, run()
takes also args=()
. Looks like that needs to be passed to the callback.
...bm.wala.cast.python.ml.test/source/com/ibm/wala/cast/python/ml/test/TestTensorflowModel.java
Outdated
Show resolved
Hide resolved
Only having it in the CG is not going to help us.
I don't think we need both.
One direct and one indirect.
We know this now as a result of wala#109.
This is really an interesting case with the |
(Quoting my own message here) I don't know if that's possible to say in the XML summary. We may need some Java code to unpack that. |
<new def="x" class="Lobject" /> | ||
<putfield class="LRoot" field="MirroredStrategy" fieldType="LRoot" ref="self" value="x" /> | ||
<putfield class="LRoot" field="$callback" fieldType="LRoot" ref="x" value="fn" /> | ||
<call class="LRoot" name="do" descriptor="()LRoot;" type="virtual" arg0="fn" arg1="1" numArgs="2" def="xx" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does arg1
take 1
here?
@@ -786,6 +792,58 @@ | |||
</class> | |||
</package> | |||
|
|||
<package name="tensorflow/distribute"> | |||
<class name="MirroredStrategy" allocatable="true"> | |||
<method name="do" descriptor="()LRoot;" numArgs="3" paramNames="self function"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there three arguments but only two parameter names?
OK. I think I made some kind of progress here. I'm seeing:
So, it's a tuple of tensors being sent to @tatianacv Can you verify we don't have this in the PA without my latest changes? |
Superseded by #61. |
Fixes wala#92.