gfile: conditionally import tensorflow_io #5491
Conversation
|
|
||
| def testTryToSupportTfio(self): | ||
| with mock.patch.object(tf.io, "gfile") as gfile_mock: | ||
| gfile_mock.return_value.get_registered_schemes = mock.MagicMock() |
There was a problem hiding this comment.
Is this multi-level mocking necessary? I would have though it would suffice to just do
with mock.patch.object(tf.io.gfile, "get_registered_schemes")There was a problem hiding this comment.
Removed, I initially mocked for getattr(tf.io.gfile, "get_registered_schemes", None) in the first layer, but now realized it's unnecessary.
There was a problem hiding this comment.
Made further changes in the mock due to attribute get_registered_schemes not found error: https://github.com/tensorflow/tensorboard/runs/4794613032?check_suite_focus=true
| mock_import.assert_not_called() | ||
| mock_gfile_exists.assert_called_once_with("gs://bucket/abc") | ||
|
|
||
| def testTryToSupportTfio_fallback_raiseError(self): |
There was a problem hiding this comment.
This test might be simpler if you only have one error (the UnimplementedError from exists()) and don't also include the ImportError. Otherwise, it's sort of combining multiple error conditions, which seems unnecessarily complicated?
There was a problem hiding this comment.
This simpler version was the test above, this one is meant to check the error message (with no supported schemes if fallen back). But I can remove it if the check is unnecessary.
There was a problem hiding this comment.
Ah ok, I see the reasoning now since it affects the error message.
| path: A strings representing an input log directory. | ||
| Returns: | ||
| Filesystem scheme, None if the path doesn't contain one. The filesystem | ||
| scheme is usually separated by `://` from the local filesystem path if |
There was a problem hiding this comment.
Totally optional but I'd tend to put a longer comment like this up above Args (and after the 1-liner docstring), vs hiding it inside Returns.
| @@ -30,6 +30,7 @@ | |||
| from tensorboard.plugins.pr_curve import metadata as pr_curve_metadata | |||
| from tensorboard.plugins.scalar import metadata as scalar_metadata | |||
| from tensorboard.util import tb_logging | |||
| from tensorboard.compat import tf | |||
There was a problem hiding this comment.
We should add a "//tensorboard/compat:tensorflow" dep on the corresponding BUILD target for this.
Same goes for the test file.
| mock_import.assert_not_called() | ||
| mock_gfile_exists.assert_called_once_with("gs://bucket/abc") | ||
|
|
||
| def testTryToSupportTfio_fallback_raiseError(self): |
There was a problem hiding this comment.
Ah ok, I see the reasoning now since it affects the error message.
There was a problem hiding this comment.
Thanks, generally LG aside from the build deps!
One note - if possible, it's nice if you can avoid force-pushing the PR for cases where that would discard commits that have previous comments on them, since the GitHub UI makes it difficult to respond to previous comments if their commits were already discarded. It should usually be possible to just push new commits w/ whatever changes you've made to the PR, and if you need to merge in new changes from master that can be done w/ a merge commit.
| class FileSystemSupport(tb_test.TestCase): | ||
| def testCheckFilesystemSupport(self): | ||
| with mock.patch.object(tf.io, "gfile", autospec=True) as mock_gfile: | ||
| mock_gfile.get_registered_schemes = mock.MagicMock( |
There was a problem hiding this comment.
So I think the explicit mock construction shouldn't be necessary. Re this comment:
Made further changes in the mock due to attribute get_registered_schemes not found error: https://github.com/tensorflow/tensorboard/runs/4794613032?check_suite_focus=true
From the errors there, it looks like get_registered_schemes not found is happening because this test is ending up using the TF stub rather than actually managing to import TF, since the error I see is:
AttributeError: <module 'tensorboard.compat.tensorflow_stub.io.gfile' from '...data_ingester_test.runfiles/org_tensorflow_tensorboard/tensorboard/compat/tensorflow_stub/io/gfile.py'> does not have the attribute 'get_registered_schemes'
I think the failure to import TF might be somehow related to the summary dependency issues (since those were also failing in the same CI run). Would you mind trying again with the unaffected nightly?
Also, since this means that this test does depend on a real TF in order to run properly, we should add a "//tensorboard:expect_tensorflow_installed" placeholder dep on the BUILD rule for this test.
There was a problem hiding this comment.
Done, thanks for the info!
* conditionally import tensorflow_io * refactor
* conditionally import tensorflow_io * refactor
|
Hi, Is it already released? |
|
@rhps Yes. |
* conditionally import tensorflow_io * refactor
* conditionally import tensorflow_io * refactor
Conditionally import
tensorflow_iomodule for additional cloud file system support (https://www.tensorflow.org/io). If the module is missing, prompt the user to runpip install tensorflow_io.Relevant issues:
tf.io.gfilewithtensorflow-io#5359#tensorflow_io