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 gfile api shim to remove tensorflow dependency for basic IO. #2586

Merged
merged 3 commits into from
Nov 16, 2022

Conversation

chiamp
Copy link
Collaborator

@chiamp chiamp commented Nov 4, 2022

Taking over #2073
Added a shim for tensorflow.io.gfile with io.py to remove tensorflow dependency.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@chiamp chiamp requested a review from levskaya November 4, 2022 01:10
tests/io_test.py Outdated Show resolved Hide resolved
tests/io_test.py Outdated Show resolved Hide resolved
tests/io_test.py Outdated Show resolved Hide resolved
tests/io_test.py Outdated Show resolved Hide resolved
tests/io_test.py Outdated Show resolved Hide resolved
tests/io_test.py Outdated Show resolved Hide resolved
tests/io_test.py Outdated Show resolved Hide resolved
tests/io_test.py Outdated Show resolved Hide resolved
@chiamp chiamp force-pushed the flax_io branch 3 times, most recently from 3dfa654 to e05ded2 Compare November 15, 2022 00:22
Copy link
Collaborator

@levskaya levskaya left a comment

Choose a reason for hiding this comment

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

lgtm!

@copybara-service copybara-service bot merged commit 1e7d843 into google:main Nov 16, 2022
@chiamp chiamp deleted the flax_io branch November 16, 2022 06:08
io_mode = BackendMode.TF
else:
logging.warning("Tensorflow library not found, tensorflow.io.gfile "
"operations will use slower native shim calls.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are native operations really slower than tensorflow?
Maybe worth pointing out that any access to GCS (i.e. "gs://" paths) will fail. Being explicit about this failure mode might also be a good idea inside below functions (e.g. when a program runs fine in one environment, but then crashes in another environment, it might not be obvious to the user that the 2nd invocation has crashed because it was trying to access a "gs://" path in an environment that did not have Tensorflow installed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm unsure on how the speed of native operations compare to tensorflow; maybe @levskaya can jump in.
As for GCS, yes good point, I'll add a disclaimer in the warning message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created a new issue #2624

wookayin added a commit to wookayin/flax that referenced this pull request Dec 12, 2022
This fixes up a small mistake in google#2586, where tensorflow is still being
imported from `flax.training.checkpoints`. To avoid the tensorflow
dependency, an alias `flax.io.NotFoundError` is added which can be
either `tf_errors.NotFoundError` or `FileNotFoundError` depending
on the backend mode.
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.

3 participants