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 numpy conversion for tf MirroredVariable. #18675

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Conversation

qlzh727
Copy link
Member

@qlzh727 qlzh727 commented Oct 23, 2023

This is a corner case that np.array requires the return type of array to be an array, and scalar will fail the type check.

This should address #18625

This is a corner case that np.array requires the return type of __array__
to be an array, and scalar will fail the type check.
@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2ad8e07) 78.57% compared to head (2beed31) 57.73%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #18675       +/-   ##
===========================================
- Coverage   78.57%   57.73%   -20.85%     
===========================================
  Files         335      335               
  Lines       32979    32979               
  Branches     6455     6455               
===========================================
- Hits        25913    19039     -6874     
- Misses       5510    12518     +7008     
+ Partials     1556     1422      -134     
Flag Coverage Δ
keras 57.73% <0.00%> (-20.75%) ⬇️
keras-jax ?
keras-numpy 57.73% <0.00%> (ø)
keras-tensorflow ?
keras-torch ?

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

Files Coverage Δ
keras/backend/common/variables.py 97.45% <0.00%> (-2.55%) ⬇️

... and 179 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fchollet
Copy link
Collaborator

Thanks for the PR! The test seems to be failing:

  File "/home/runner/work/keras/keras/integration_tests/tf_distribute_training_test.py", line 77, in <module>
    test_model_fit()
  File "/home/runner/work/keras/keras/integration_tests/tf_distribute_training_test.py", line 54, in test_model_fit
    history = model.fit(
  File "/home/runner/work/keras/keras/keras/utils/traceback_utils.py", line 123, in error_handler
    raise e.with_traceback(filtered_tb) from None
  File "/home/runner/work/keras/keras/keras/backend/common/variables.py", line 186, in __array__
    return np.asarray(self.value, dtype)
ValueError: object __array__ method not producing an array

@qlzh727
Copy link
Member Author

qlzh727 commented Oct 23, 2023

Thanks for the PR! The test seems to be failing:

  File "/home/runner/work/keras/keras/integration_tests/tf_distribute_training_test.py", line 77, in <module>
    test_model_fit()
  File "/home/runner/work/keras/keras/integration_tests/tf_distribute_training_test.py", line 54, in test_model_fit
    history = model.fit(
  File "/home/runner/work/keras/keras/keras/utils/traceback_utils.py", line 123, in error_handler
    raise e.with_traceback(filtered_tb) from None
  File "/home/runner/work/keras/keras/keras/backend/common/variables.py", line 186, in __array__
    return np.asarray(self.value, dtype)
ValueError: object __array__ method not producing an array

ok. Now it should be fixed.

Copy link
Collaborator

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Oct 24, 2023
@fchollet fchollet merged commit fd48a49 into keras-team:master Oct 24, 2023
6 checks passed
@google-ml-butler google-ml-butler bot removed awaiting review ready to pull Ready to be merged into the codebase kokoro:force-run labels Oct 24, 2023
@qlzh727 qlzh727 deleted the fix branch October 24, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants