Warn when creating driver that points to an empty directory #77

pzdkn · 2022-08-04T10:15:45Z

Description

A warning is shown when we iterate over a driver, that points to an empty or non-existent directory.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactoring including code style reformatting
Other (please describe):

Checklist:

I have read the contributing guideline doc (external contributors only)
Lint and unit tests pass locally with my changes
I have kept the PR small so that it can be easily reviewed
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
All dependency changes have been reflected in the pip requirement files.

squirrel/fsspec/fs.py

squirrel/driver/store_driver.py

squirrel/fsspec/fs.py

test/test_driver/test_msgpack.py

squirrel/fsspec/fs.py

AlpAribal · 2022-08-09T09:08:43Z

Shall we base this branch onto #72, there are some common changes in both, it might be safer to see how the changes combined work together.

AlirezaSohofi · 2022-08-09T12:32:43Z

Shall we base this branch onto #72, there are some common changes in both, it might be safer to see how the changes combined work together.

agree, can you please rebase this branch please @pzdkn?

pzdkn · 2022-08-10T12:57:34Z

I have rebased and tests for both branches pass. So we can merge #72 first, and then take care of this one @AlirezaSohofi @AlpAribal.

test store creation with empty path and non-empty path create directory when inserting shards Update squirrel/store/squirrel_store.py Co-authored-by: Alireza Sohofi <a.sohofi@gmail.com> Update test/test_driver/test_sq_store.py Co-authored-by: Alp Arıbal <AlpAribal@users.noreply.github.com> move dir-exist check out of set operation add set operation in test; adapt dir exist flag, because dir doesnt exist after clean remove created_dir_if_not_exist function bump minor version throw error when instantiating a driver that points to an empty or invalid url refactor warning into FilePathGenerator

AlpAribal · 2022-08-16T06:41:03Z

squirrel/iterstream/source.py

+                if not self._returned_url:
+                    self._returned_url = True


We need the same check for the nested case as well. I think, right now, we will get an empty warning for a dir containing only dirs but no files

Do we want to cover the nested case though? We are not reading in a nested way in the first place.

FilePathGenerator is not only menat to be used in SquirrelStore, so it should be general and and able to handle nested cases too.

AlpAribal · 2022-08-16T06:45:05Z

test/test_driver/test_msgpack.py

+            with warnings.catch_warnings():
+                warnings.simplefilter("ignore")
+                _ = MessagepackDriver(url=tmp_dir).get_iter().collect()


Why do we catch/filter warnings here?

This is to check that when there are items in the store, then no warnings shall be printed.
This .simplefilter asserts that no warning is shown:

https://stackoverflow.com/questions/45671803/how-to-use-pytest-to-assert-no-warning-is-raised

But it is in fact unintuitive, since a directory containing empty directories will no yield warnings

AlpAribal · 2022-08-16T06:46:04Z

test/test_driver/test_msgpack.py

+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")


Why do we catch/filter warnings here?

AlpAribal · 2022-08-16T06:46:32Z

test/test_driver/test_msgpack.py

@@ -10,6 +11,34 @@
 from squirrel.store import SquirrelStore


+def test_invalid_url(local_msgpack_url: URL) -> None:


Would rename this test, it does not only use invalid urls.

We need to add nested cases here, perhaps via parametrize

AlirezaSohofi · 2022-08-17T16:36:58Z

squirrel/iterstream/source.py

-                            yield f"{self.protocol}{url}"
+                            if not self._returned_url:
+                                self._returned_url = True
+                                yield f"{self.protocol}{url}"


Suggested change

yield f"{self.protocol}{url}"

yield f"{self.protocol}{url}"

AlirezaSohofi · 2022-08-23T15:32:24Z

squirrel/iterstream/source.py

+                if not self._returned_file_url and not self.fs.isdir(path=url):
+                    self._returned_file_url = True


Suggested change

if not self._returned_file_url and not self.fs.isdir(path=url):

self._returned_file_url = True

if not self._returned_file_url:

self._returned_file_url = True

why do we need not self.fs.isdir(path=url)?

pzdkn requested review from AlpAribal and AlirezaSohofi August 4, 2022 10:17

AlirezaSohofi suggested changes Aug 4, 2022

View reviewed changes

squirrel/fsspec/fs.py Outdated Show resolved Hide resolved

squirrel/driver/store_driver.py Outdated Show resolved Hide resolved

pzdkn requested a review from AlirezaSohofi August 5, 2022 12:20

AlirezaSohofi suggested changes Aug 5, 2022

View reviewed changes

squirrel/fsspec/fs.py Outdated Show resolved Hide resolved

test/test_driver/test_msgpack.py Show resolved Hide resolved

squirrel/fsspec/fs.py Outdated Show resolved Hide resolved

pzdkn requested a review from AlirezaSohofi August 8, 2022 17:14

pzdkn force-pushed the peng-warn-about-empty-url branch from 8dca81c to dcc60a5 Compare August 9, 2022 17:38

pzdkn changed the title ~~Throw error when creating driver that points to an empty directory~~ Warn when creating driver that points to an empty directory Aug 10, 2022

pzdkn changed the base branch from main to peng-store-create-path-when-non-existent August 12, 2022 09:44

Base automatically changed from peng-store-create-path-when-non-existent to main August 12, 2022 09:53

pzdkn force-pushed the peng-warn-about-empty-url branch from 6086900 to f71a756 Compare August 15, 2022 16:11

bump patch version; correct docstring

1655c19

AlpAribal suggested changes Aug 16, 2022

View reviewed changes

pzdkn added 2 commits August 17, 2022 18:13

add warning for nested dir-structure too

c698938

distinguish warnings between empty url and invalid url; add more tests

e42a07b

AlirezaSohofi reviewed Aug 17, 2022

View reviewed changes

pzdkn added 2 commits August 17, 2022 18:41

fix bug

66f2513

change python version for precommit

47556e3

pzdkn requested review from AlirezaSohofi and AlpAribal August 18, 2022 07:49

AlirezaSohofi suggested changes Aug 23, 2022

View reviewed changes

pzdkn closed this Sep 7, 2022

github-actions bot locked and limited conversation to collaborators Sep 7, 2022

pzdkn deleted the peng-warn-about-empty-url branch September 7, 2022 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when creating driver that points to an empty directory #77

Warn when creating driver that points to an empty directory #77

pzdkn commented Aug 4, 2022 •

edited

Loading

AlpAribal commented Aug 9, 2022

AlirezaSohofi commented Aug 9, 2022

pzdkn commented Aug 10, 2022 •

edited

Loading

AlpAribal Aug 16, 2022

pzdkn Aug 16, 2022

AlirezaSohofi Aug 17, 2022

AlpAribal Aug 16, 2022

pzdkn Aug 16, 2022

pzdkn Aug 16, 2022

AlpAribal Aug 16, 2022

AlpAribal Aug 16, 2022

AlpAribal Aug 16, 2022

AlirezaSohofi Aug 17, 2022

AlirezaSohofi Aug 23, 2022

AlirezaSohofi Aug 23, 2022

		with warnings.catch_warnings():
		warnings.simplefilter("ignore")

		@@ -10,6 +11,34 @@
		from squirrel.store import SquirrelStore


		def test_invalid_url(local_msgpack_url: URL) -> None:

		if not self._returned_file_url and not self.fs.isdir(path=url):
		self._returned_file_url = True

Warn when creating driver that points to an empty directory #77

Warn when creating driver that points to an empty directory #77

Conversation

pzdkn commented Aug 4, 2022 • edited Loading

Description

Type of change

Checklist:

AlpAribal commented Aug 9, 2022

AlirezaSohofi commented Aug 9, 2022

pzdkn commented Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzdkn commented Aug 4, 2022 •

edited

Loading

pzdkn commented Aug 10, 2022 •

edited

Loading