-
Notifications
You must be signed in to change notification settings - Fork 354
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: keras import errors in new versions #9141
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9141 +/- ##
==========================================
- Coverage 46.54% 42.20% -4.34%
==========================================
Files 755 1147 +392
Lines 104838 143603 +38765
Branches 2413 2413
==========================================
+ Hits 48795 60612 +11817
- Misses 55834 82782 +26948
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more.
|
c7f0510
to
17f3882
Compare
harness/determined/keras/_load.py
Outdated
|
||
import determined as det | ||
from determined import keras | ||
|
||
# In TF 2.14, tracking module was removed. | ||
if version.parse(tf.__version__) >= version.parse("2.14.0"): | ||
from tensorflow.python.trackable.autotrackable import AutoTrackable # noqa: I2041 |
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.
we have a code style guideline where we don't want to use object names directly without modules.
Can you change both of these imports to something like:
from tensorflow.python.trackable import autotrackable as tf_tracking
from tensorflow.python.training.tracking import tracking as tf_tracking
Then you can reference the object as tf_tracking.AutoTrackable
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.
Thanks. I hadn't consider renaming the module, so I can now use new_module_name.object
as the type and don't need noqa: I2041
in importing the current version tensorflow.
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.
For the test plan, I think it's sufficient to test only the TensorFlow image changes based on what is changed in this PR.
I would update the test plan to test the NGC image and the 2.12 TF image.
17f3882
to
1bf8fb0
Compare
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.
LGTM
Ticket
MD-358
Description
Update the import paths of some modules in tensorflow and Keras.
Test Plan
Create experiments and see if they complete successfully:
example/computer_vision/iris_tf_keras/const.yaml
: Use determinedai/tensorflow-ngc:748dda4 in the experiment config.example/computer_vision/iris_tf_keras/const.yaml
: Use determinedai/tensorflow-ngc-hpc:748dda4 in the experiment config.Checklist
docs/release-notes/
.See Release Note for details.