add test for filesystem plugins

[vnghia]

I can not merge this PR because of #1222 .

I would like to share with you how I think we should test the plugins. The idea is inspired by modular_filesystem_test. I used parameterized tests for control the plugins and environements variable. Please let me know what you think !

Part of #1183

[yongtang]

@vnvo2409 parameterized pytest looks good! This can help standardize the file system implementations.

[vnghia]

@yongtang
Could you please take a look ? I wrote it with a super flexibility. However, I am not sure if I am overdoing it or not. Thank you !
( This test requires a new feature from pytest==6.2.0)

[vnghia]

One more thing. I find it hard to test the plugins with environment variables. Because most filesystems like s3 will only check for the envs when initialization.

I am thinking of introduce a new env S3_FORCE_REINITIALIZATION ( we will warn that it is for testing only). Everytime this env is present, everything will be reinitialized by GetTransferManager, GetExecutor and GetS3Client and so on for other filesystem. Another way is reload(tensorflow). WDYT ?

[vnghia]

@yongtang

Do you have any thought about this approach ? Thank you !

[yongtang]

@vnvo2409 I think one concern is that when you change the configuration/initialization, the change applies process wise - meaning all current ongoing pytests (within the same process) will be impacted. Unless we can force pytest to run in a sequence order then there might have unintended effect. One test belonging to one group may get into the way of another test belonging to another group (with different intended settings)?

[vnghia]

@yongtang

I am trying to elaborate what I am understand. In summary, there are 2 layers of isolation ( explained below, you could see that I don't use any os.environ for setting env ). So I think these tests won't clash with each other. Please tell me if there are something unclear.

[vnghia]

@yongtang. Since #1222 is done. We could test the plugins with TF nightly, could you take a look at it ? If it is ok, I will expand for all operations ( not only reading/writing )

[yongtang]

@vnvo2409 Sounds good to me 👍. Let me know when the PR draft is ready

[vnghia]

@yongtang Sorry for putting this PR on hold for too long. Could you take a look please ? I will add some more tests ( mostly directory-related ) in later PRs. Thank you !

In addition, I've already exposed env through tensorflow/tensorflow#46302. Could we update gcs right now or we have to wait for tensorflow 2.5rc0 ?

[yongtang]

@vnvo2409 The PR looks good! There are a couple of test failures on Linux, can you take a look (or disable it temporarily for now)?

[yongtang]

@vnvo2409 For gcs the plan is to enable modular file system (and use TF_ENABLE_LEGACY_FILESYSTEM=1 to switch on/off) before the 2.5 branch and 2.5.0rc0 is out. Though I think we want to send out the email to community (tf developer google group) first so that people is prepared. /cc @mihaimaruseac

[vnghia]

@yongtang with gcs, I means updating the gcs_env

io/tensorflow_io/core/plugins/gs/gcs_env.cc

Lines 82 to 85 in ac75e1c

	uint64_t GCSNowSeconds(void) {
	// TODO: Either implement NowSeconds here, or have TensorFlow API exposed
	std::abort();
	}

Nothing is related to the release plan I think. But we need the env header right now is exposed in tf-nightly only so I am not sure if we have to wait for tf-2.5rc0

[yongtang]

@vnvo2409 tensorflow-io is already using tf-nightly now (see https://github.com/tensorflow/io/blob/master/tensorflow_io/core/python/ops/version_ops.py), which was added as part of PR #1309

[vnghia]

@yongtang I am so sorry that I will have to put it on hold for much longer. It has some weird errors and I don't have a linux machine right now. I will probably try it again when tf 2.5 is out and aws and google-cloud-cpp are removed from TensorFlow.

[vnghia]

@yongtang It seems that the errors are from the upstream. I would like to ask if it is possible to switch to use google-cloud-cpp bigtable and pubsub client instead of implementing our own client with our own proto client ? I could give it a try if you think it's ok.

[yongtang]

I would like to ask if it is possible to switch to use google-cloud-cpp bigtable and pubsub client instead of implementing our own client with our own proto client ? I could give it a try if you think it's ok.

@vnvo2409 That will be great and preferred 👍

[vnghia]

@yongtang This PR is ready to merge. Turn out azurite was too old and it broke the tests.

The philosophy of the test is still the same. But the filesystem are added manually instead of using parameterized tests which allows greater control and less generated tests. We can also write test for a specific filesystem and reuse the resources created in *_fs.

Still, testing with different envs right now aren't possible as the envs are read only once when the filesystem are loaded for the first time. It will be addressed along with #1342 and #1354.

[yongtang]

LGTM. A couple of minor cosmetic comments though those can be updated in follow up PRs.