-
Notifications
You must be signed in to change notification settings - Fork 706
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
Transform fails when setting force_tf_compat_v1=False #3272
Comments
Can you share with us statistics about this feature from StatisticsGen? |
@zoyahav This is an extremely small test dataset, ~300 data points, so min=max=average length=19 and no items are missing, as is confirmed by output from StatisticsGen. As mentioned, this works perfectly when setting |
@zoyahav This is the output from stats gen: |
Thanks! I am looking into this. Will provide an update when I have a fix. |
@varshaan Thats great! Let me know if you need anything else from me. |
@varshaan Any update on this? |
@varshaan That's great! Our pipelines are still on TFX 0.27.0 and isn't fully TFX >= 0.28.0 compatible. I will try to bump TFX and try your fix as soon as I can and get back with the result. |
@varshaan Now I've managed to test this and unfortunately the issue seems to remain for me.
And if I run the exact same code and data with 0.29.0rc0 I get an error that seems very similar to the one I first posted:
I would be very appreciative if you could have another look at this! |
@varshaan Any update on this? |
Sorry, I missed your previous comment. Looking into this now. |
No worries, thanks for investigating! |
@varshaan Any update? Have you been able to reproduce? |
Yes, I was able to repro. I have a change in progress to address this. I will let you know once its submitted. |
Great, thanks! |
Just leaving an update. I'm still working on fully testing this as the change is a bit non-trivial. I will try and get it submitted soon. |
@varshaan Thanks for the update! |
@varshaan Any update on this? Has your fix made it into the 1.0.0rc1 release? |
I have the exact same issue in the 1.0.0rc1 release, upgrading from 0.27.0. |
This commit is supposed to address this: tensorflow/transform@b878c66 It is not in the 1.0, but it should be in the next release. You can try testing with a nightly build post that commit if possible. |
@varshaan Transform nightly version 1.1.0.dev20210617 works for me! Great job! @axeltidemann Do you have the possibility to test this as well? |
Yes, this works for me using TFX 1.1.0.dev20210617. |
Thanks for verifying. I will close this issue. |
I believe this issue has resurfaced for me in TFX 1.3.0. @axeltidemann Is it the same for you? @varshaan Has there been an regression in the latest TFX releases? |
@ConverJens I don't know for 1.3.0, I am on TFX 1.2.0 which does not have this issue. |
There should be no regression that I am aware of. The regression tests I added with the previous fix still pass. Could you provide a minimal repro so I can test it? |
@varshaan Thanks for the quick response! I will test it a bit more and, if the issue persists, provide you with a minimal example. |
@varshaan I've able to reproduce the issue now with TFX 1.4.0. It appears that the issue reappears when one sets I'm attaching a notebook (with data generation) which reproduces the issue locally for me. |
Thanks for the repro! I'll take a look at this and get back to you by end of week. |
This issue reproduces as far back as TFX 1.2 if disable_statistics=True, so this isn't a regression. The original fix missed something in this code path. Thanks for reporting, I will send out a fix for this and update here after that. |
@varshaan Great, thanks! |
@varshaan Do you have an update for this issue? |
Hi, yes, I debugged this further and while the error message is the same, the source of the errors is different. It'll take me a couple more weeks to figure out exactly how to address what's happening in the disable_statistics=True case. Sorry about that, but I hope to have a fix by end of this month. |
@varshaan No worries! Great work and thanks for the update! |
An update, I have a change in review to address this. Will comment back here once it is submitted. |
Great! I'll happily test it as soon as a nightly build with the fix is out. |
Hi, the commit to address this was submitted earlier today [1]. Feel free to re-open this issue if you find that that didn't solve your problem. |
@varshaan This issue now seems to be resolved. Thank you very much for your help! |
System information - Have I specified the code to reproduce the issue
(Yes/No): Yes
Describe the current behavior
Using to tf.strings.substr operations in the preprocessing fn used in Transform when setting
force_tf_compat_v1=False
fails while running in KubeFlow. Settingforce_tf_compat_v1=True
works.Note: running in interactive mode, using
force_tf_compat_v1=True
causes python to crash and hence can't be tested properly.Describe the expected behavior
Use native tf2 behaviour should work.
Standalone code to reproduce the issue Providing a bare minimum test case or
step(s) to reproduce the problem will greatly help us to debug the issue. If
possible, please share a link to Colab/Jupyter/any notebook.
Using attached file as preprocessing in KubeFlow with data generated by running (transform.py.zip):
Other info / logs
This is the operation that I'm using in my preprocessing fn:
Running locally this works in both eager and graph mode by setting
tf.config.run_functions_eagerly(False/True)
.However when running it through the Transform component and setting
force_tf_compat_v1=False
it fails with the following message:When setting
force_tf_compat_v1=False
it works as expected.The text was updated successfully, but these errors were encountered: