[BugFix] Fixes #20983 - Store py_func dtypes for correct conversion #21038
Conversation
|
^ WIP on the CI errors - they were due to the My fixes are done, I am in-tow for the 9 million bazel files that needed recompilation :) Will squash and push once I confirm things are resolved locally. |
sabhiram
force-pushed
the
dev-recogni/bug_20983
branch
from
68e11a5
to
1e0bbc9
Compare
| """Tests for script_ops.py_func.""" | ||
|
|
||
| def test_basic_convert(self): | ||
| def get_not_empty_list(args): |
There was a problem hiding this comment.
Can you delete this file and add your tests to tensorflow/python/kernel_tests/py_func_test.py?
tensorflow/python/BUILD
Outdated
| @@ -2524,6 +2524,18 @@ py_library( | |||
| ], | |||
| ) | |||
|
|
|||
| py_test( | |||
There was a problem hiding this comment.
Delete this build rule and instead add your tests to tensorflow/python/kernel_tests/py_func_test.py
There was a problem hiding this comment.
Can do - thanks for the pointers. Will fix and push shortly.
…nversion
1. Implement wrapper class `PythonFunc` to store Tout list
for appropriate dtype conversion from tensorflow types to
numpy types using the `dtype.as_numpy_dtype` member.
2. Adjust BUILD file to include py_test of the script_ops_test.
3. Add script_ops_test to illustrate broken behavior (see
`test_convert_empty_list`). Run the specific test suite with:
bazel test //tensorflow/python:script_ops_test
4. `_convert` was being called from dataset_ops, update this to
refer to the `PythonFunc` instead of the `FuncRegistry`.
5. [Review Feedback] Move script_ops tests to py_func_test.py. Also
removed the BUILD rules for the deleted test file.
sabhiram
force-pushed
the
dev-recogni/bug_20983
branch
from
1e0bbc9
to
29831cd
Compare
|
@akshayka - Thanks for the feedback, I think I should have done justice to your comments. Please let me know if this looks better. PS: One of the basic tests that I had originally added seemed useless given the py_func_test.py file, so I went ahead and removed it (preserved the test that will fail without these changes). |
tensorflow/python/ops/script_ops.py
Outdated
| self._convert(x, dtype=dtype.as_numpy_dtype) | ||
| for (x, dtype) in zip(ret, self._out_dtypes) | ||
| ] | ||
| return self._convert(ret) # TODO: pass self._out_dtypes? |
There was a problem hiding this comment.
Is there any reason to not pass out_dtypes here?
There was a problem hiding this comment.
I suppose if the py_func returns a None, it could lead to some similar (unexpected) behavior. Let me see about adding another test or two so we can check this. 👍
There was a problem hiding this comment.
One thing - the registry will force the Tout to be an array / list -- see ~line 270 in the _internal_py_func where the Tout is initialized. So we would need to sanitize the _out_dtypes. But that should be easy.
Disabled the `testBadNumpyReturnType` which should allow for a conversion of an arbitrary np.array into a list of a specified dtype. Modified the exception expectation for the `testBadReturnType` test.
|
@akshayka - One of the tests is commented out as (I think) it no longer applies. Once we are all happy with this changeset, I can squash the edits so you have a single commit to merge (if that matters). |
|
Great, go ahead and delete the test you commented out, I agree with you that it no longer applies. |
|
@akshaym - any reservations? |
|
@akshayka - I think this is good to go - might need your blessing before the bot takes over. |
PiperOrigin-RevId: 208232199
|
@akshayka - Heh, what did I break? Unfortunately I cannot peer into the CI process enough to guess how this affected what. Happy to help see it through if needed. |
|
Hey Sabhiram, sorry for the delay. I didn't have the time to diagnose the issue, but the change caused an internal-only test to segfault. |
PythonFuncto store Tout listfor appropriate dtype conversion from tensorflow types to
numpy types using the
dtype.as_numpy_dtypemember.testConvertEmptyList).