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

Add a warning when fitting models loaded from SavedModels #42846

Merged
merged 7 commits into from Jun 10, 2021

Conversation

bhack
Copy link
Contributor

@bhack bhack commented Aug 31, 2020

Add a warning when fitting models loaded from SaveModel having optimizers with slots.
Ref: #42749 (comment)

Add a warning when fitting models loaded from SaveModel having optimizers  with slots.
Ref: tensorflow#42749 (comment)
@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Aug 31, 2020
@gbaned gbaned self-assigned this Sep 1, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 1, 2020
@gbaned gbaned added the comp:keras Keras related issues label Sep 1, 2020
@bhack
Copy link
Contributor Author

bhack commented Sep 1, 2020

/cc @reedwm @k-w-w

@bhack bhack mentioned this pull request Sep 1, 2020
17 tasks
@bhack
Copy link
Contributor Author

bhack commented Sep 1, 2020

This is just an exploring proposal. If you have a better entry point let me know.

@gbaned
Copy link
Contributor

gbaned commented Nov 18, 2020

@bhack @reedwm @k-w-w This PR is in draft, any update on this? Please. Thanks!

@bhack
Copy link
Contributor Author

bhack commented Nov 18, 2020

@gbaned I have any private channel so the thread it is what you see here.

@reedwm
Copy link
Member

reedwm commented Dec 2, 2020

Can you give context for this PR? I think adding a warning in fit() for loaded saved models with slot variables is sensible, but I don't see any warnings added in this PR.

@bhack
Copy link
Contributor Author

bhack commented Dec 2, 2020

@reedwm If you see is a draft as I have asked for comment on 1st September about the entry point I selected in the code to "introduce" the condition but as of 2nd December I got any feedback.
I have any other chat channel to discuss with devs as Gitter SIG Keras-io/Lobby in the current status is useless for discussion about PRs.

@reedwm reedwm requested a review from k-w-w December 2, 2020 19:01
@reedwm
Copy link
Member

reedwm commented Dec 2, 2020

Unfortunately I am not very familiar with the SavedModel code so I'm not sure if this is a good place to do it. Sorry for not responding until now.

@k-w-w, do you think it would be a good idea to add a warning if a SavedModel with slot variables is restored and Model.fit() is used? Currently slot variables are not restored in such a case, which could affect training and is why we should potentially add a warning. If this is a good idea, would the code modified in this PR be a good place to check for the use of slot variables?

@bhack
Copy link
Contributor Author

bhack commented Dec 2, 2020

@reedwm Thank you. Just to note that I've already asked to @k-w-w on 23th Oct at tensorflow/community#281 (comment)

@k-w-w
Copy link
Contributor

k-w-w commented Dec 2, 2020

The change looks good, although I think directly sending the warning when loading would make sense (see the warning message for the HDF5 format:

'your model is starting with a freshly initialized optimizer.')
)

@bhack
Copy link
Contributor Author

bhack commented Dec 2, 2020

The change looks good, although I think directly sending the warning when loading would make sense (see the warning message for the HDF5 format:

'your model is starting with a freshly initialized optimizer.')

)

Do you like the same HDF5 warning message or something else?

@bhack bhack marked this pull request as ready for review December 2, 2020 22:34
@bhack
Copy link
Contributor Author

bhack commented Dec 3, 2020

Ok Windows is failing for unrelated MLIR issues and Macos CPU is failing probably for other unrelated stuff but has any visible details.
The other checks are passing.

@k-w-w k-w-w added the kokoro:force-run Tests on submitted change label May 27, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 27, 2021
Copy link
Contributor

@k-w-w k-w-w left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, thank you!

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 27, 2021
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer May 27, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 27, 2021
@bhack
Copy link
Contributor Author

bhack commented May 27, 2021

@k-w-w The sanity is temp broken by #48294 (comment)

@k-w-w
Copy link
Contributor

k-w-w commented May 27, 2021

@k-w-w The sanity is temp broken by #48294 (comment)

Oh I see, good to know

@bhack
Copy link
Contributor Author

bhack commented May 27, 2021

Can you re-trigger the build? I think that the CI Sanity it is working again.

@rthadur rthadur added the kokoro:force-run Tests on submitted change label May 27, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 27, 2021
@k-w-w k-w-w added the kokoro:force-run Tests on submitted change label May 28, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 28, 2021
@gbaned gbaned removed the awaiting review Pull request awaiting review label May 28, 2021
@bhack
Copy link
Contributor Author

bhack commented Jun 7, 2021

Do you need anything else here?

Copy link
Contributor

@k-w-w k-w-w left a comment

Choose a reason for hiding this comment

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

Hmm I'm not sure why this isn't being automatically pulled in. Let me try re-approving.

@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Jun 7, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 7, 2021
@bhack
Copy link
Contributor Author

bhack commented Jun 7, 2021

@k-w-w If you want you can add a reference somewhere to #44670

@gbaned gbaned removed the ready to pull PR ready for merge process label Jun 10, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 10, 2021
@kokoro-team kokoro-team removed kokoro:force-run Tests on submitted change labels Jun 10, 2021
@copybara-service copybara-service bot merged commit 5dfeb87 into tensorflow:master Jun 10, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Jun 10, 2021
@bhack bhack deleted the patch-6 branch June 10, 2021 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:keras Keras related issues ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants