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

chore: replace os.path.join with tf.io.gfile.join #15551

Merged
merged 2 commits into from
Nov 11, 2021

Conversation

adriangb
Copy link
Contributor

Partially addresses #15300

I only replaced some uses of os.path.join.
I didn't want to replace it where not needed (it is only needed in things that interact ram:// or gcs:// filesystems). But I probably replied it in some places where it's not needed and neglected to replace it in places where it is needed. The codebase is very large and I'm unfamiliar with a lot of it; I'd appreciate some guidance from someone with more global knowledge of the codebase.

@google-cla
Copy link

google-cla bot commented Oct 26, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Oct 26, 2021
@adriangb
Copy link
Contributor Author

@gbaned can you kick off CI please?

I'll give this another pass in the next couple days, I can already see plenty of places I don't think replacement was needed

@adriangb
Copy link
Contributor Author

adriangb commented Nov 3, 2021

I went through this by eye. The main change I made was to revert (i.e. make no changes) to callbacks.py. It seemed like most of the path joins there were to write out logs, and it didn't seem like the ram:// or gs:// filesystems would be valid in that context. Please correct me if this is wrong.

I think all of the replacements left now directly pertain to SavedModel, which is the context where ram:// and gs:// are certainly valid and this change makes sense.

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, thank you

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Nov 9, 2021
@copybara-service copybara-service bot merged commit 59dd152 into keras-team:master Nov 11, 2021
@google-ml-butler google-ml-butler bot removed awaiting review ready to pull Ready to be merged into the codebase labels Nov 11, 2021
@adriangb adriangb deleted the replace-os-path-join branch November 11, 2021 07:07
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.

4 participants