Skip to content
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

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

jgongd
Copy link
Contributor

@jgongd jgongd commented Apr 10, 2024

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:

  1. example/computer_vision/iris_tf_keras/const.yaml: Use determinedai/tensorflow-ngc:748dda4 in the experiment config.
  2. example/computer_vision/iris_tf_keras/const.yaml: Use determinedai/tensorflow-ngc-hpc:748dda4 in the experiment config.

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@jgongd jgongd requested a review from a team as a code owner April 10, 2024 16:23
@cla-bot cla-bot bot added the cla-signed label Apr 10, 2024
Copy link

netlify bot commented Apr 10, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 1bf8fb0
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/661838c228f29b00087e6415
😎 Deploy Preview https://deploy-preview-9141--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 42.20%. Comparing base (d0497da) to head (1bf8fb0).
Report is 1 commits behind head on main.

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              
Flag Coverage Δ
backend 43.76% <ø> (+9.51%) ⬆️
harness 49.53% <60.00%> (-13.41%) ⬇️
web 36.76% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
harness/determined/keras/_load.py 87.50% <80.00%> (-1.14%) ⬇️
harness/determined/keras/_tf_keras_trial.py 81.09% <40.00%> (-0.48%) ⬇️

... and 591 files with indirect coverage changes

@jgongd jgongd force-pushed the jgong/fix/keras-cant-import-some-modules branch 2 times, most recently from c7f0510 to 17f3882 Compare April 11, 2024 01:33

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
Copy link
Contributor

@MikhailKardash MikhailKardash Apr 11, 2024

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

Copy link
Contributor Author

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.

Copy link
Contributor

@MikhailKardash MikhailKardash left a 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.

@jgongd jgongd force-pushed the jgong/fix/keras-cant-import-some-modules branch from 17f3882 to 1bf8fb0 Compare April 11, 2024 19:23
Copy link
Contributor

@MikhailKardash MikhailKardash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jgongd jgongd merged commit 291565b into main Apr 11, 2024
76 of 88 checks passed
@jgongd jgongd deleted the jgong/fix/keras-cant-import-some-modules branch April 11, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants