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 wrong path separator in error message when saved model does not exist #44741

Conversation

spopovru
Copy link
Contributor

Error message generation changed for keras.models.load_model (fixed bug #44739).
Path separator recived from os.path.sep, and now for Windows error message looking like
OSError: SavedModel file does not exist at: C:\IncorrectPathToSavedModel\{saved_model.pbtxt|saved_model.pb}
for Linux -
OSError: SavedModel file does not exist at: ~/IncorrectPathToSavedModel/{saved_model.pbtxt|saved_model.pb}

@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Nov 10, 2020
@google-cla
Copy link

google-cla bot commented Nov 10, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Nov 10, 2020
@google-cla
Copy link

google-cla bot commented Nov 10, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 10, 2020
@spopovru spopovru changed the title Fix wrong path separator in error message when saved model does not exist. #44739 Fix wrong path separator in error message when saved model does not exist Nov 10, 2020
@gbaned gbaned self-assigned this Nov 11, 2020
@gbaned gbaned added comp:keras Keras related issues prtype:bugfix PR to fix a bug labels Nov 11, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Nov 11, 2020
@gbaned gbaned self-requested a review November 11, 2020 03:23
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 11, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 11, 2020
@gbaned gbaned removed the ready to pull PR ready for merge process label Nov 11, 2020
Copy link
Member

@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.

Looks good, please add a quick unit test.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Nov 12, 2020
@bhack
Copy link
Contributor

bhack commented Nov 12, 2020

Looks good, please add a quick unit test.

Yes I think that it is better to have a quick unit test if he have enough time but also it is really trivial and it will be quite hard to create a regression there after this change.

@spopovru
Copy link
Contributor Author

Looks good, please add a quick unit test.

Thank you!
I added a test for this bug, hopefully now all right.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 12, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 12, 2020
@bhack
Copy link
Contributor

bhack commented Nov 12, 2020

/cc @rthadur Let's check if #43754 (comment) is still an issue if @fchollet will approve this without any extra commit request.

@rthadur rthadur removed the ready to pull PR ready for merge process label Nov 12, 2020
@spopovru
Copy link
Contributor Author

@fchollet can you approve this pull request, please?
I was resolve issue and add simple unit test.
It's not seriously bug, off course, but this fix make project better, imho.

@gbaned gbaned added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Dec 16, 2020
Copy link
Member

@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

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 11, 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 Mar 11, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 11, 2021
@copybara-service copybara-service bot merged commit eb76bcf into tensorflow:master Mar 12, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Mar 12, 2021
@gbaned gbaned removed the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Mar 12, 2021
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 prtype:bugfix PR to fix a bug 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

6 participants