Make FSFile hashable again #1606

gerritholl · 2021-03-19T13:54:22Z

FSFile objects had been inadvertently made unhashable by
#1582 . Make FSFile objects
hashable again by implementing a hash method.

Closes group_files does not support FSFile objects (TypeError: unhasshable type 'FSFile') #1604
Closes Reading FSFile fails with TypeError #1605
Tests added

FSFile objects had been inadvertently made unhashable by pytroll#1582 . Make FSFile objects hashable again by implementing a __hash__ method. - [x] Fixes pytroll#1604 - [x] Fixes pytroll#1605

gerritholl · 2021-03-19T13:55:50Z

Question: we should probably make FSFile immutable, but this appears easier said than done. I tried adding a __slots__ = [] attribute, following the recommendation from https://stackoverflow.com/a/4828108/974555, but this appeared to have no effects. Do we need to ensure this class is immutable or can we trust the user to behave sanely?

Remove blank line after docstring to meet PEP 257 requirements as requested by CI.

In test_init_with_fsfile, when capturing the ValueError for "no supported files found", more precisely capture the error message Satpy reports in this situation. Also remove a spurious # in the middle of a comment line.

djhoese · 2021-03-19T14:07:04Z

My hope is that with some extra work in fsspec that we won't need FSFile at all and can pass fsspec File objects (or URLs) directly to xr.open_dataset. I'd like to avoid implementing anything that isn't in the plans for fsspec. That's just my opinion though.

codecov · 2021-03-19T14:07:59Z

Codecov Report

Merging #1606 (a03208d) into master (2286d24) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
+ Coverage   92.35%   92.43%   +0.08%     
==========================================
  Files         254      254              
  Lines       37409    37500      +91     
==========================================
+ Hits        34548    34663     +115     
+ Misses       2861     2837      -24

Flag	Coverage Δ
behaviourtests	`4.77% <8.69%> (-0.01%)`	⬇️
unittests	`92.57% <100.00%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
satpy/readers/__init__.py	`97.50% <100.00%> (+0.50%)`	⬆️
satpy/tests/test_readers.py	`99.22% <100.00%> (+0.01%)`	⬆️
satpy/tests/test_scene.py	`99.44% <100.00%> (+<0.01%)`	⬆️
satpy/tests/test_data_download.py	`100.00% <0.00%> (ø)`
satpy/readers/seviri_l1b_native_hdr.py	`100.00% <0.00%> (ø)`
satpy/tests/writer_tests/test_ninjotiff.py	`100.00% <0.00%> (ø)`
satpy/tests/reader_tests/test_seviri_l1b_native.py	`99.44% <0.00%> (+0.03%)`	⬆️
satpy/readers/seviri_l1b_native.py	`79.56% <0.00%> (+0.45%)`	⬆️
satpy/aux_download.py	`97.43% <0.00%> (+9.40%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2286d24...a03208d. Read the comment docs.

gerritholl · 2021-03-19T14:20:54Z

Although this works with a LocalFileSystem, it does not work with a CachingFileSystem, because the latter is not hashable:

$ cat cfs-hash.py
from fsspec.implementations.cached import CachingFileSystem
from fsspec.implementations.local import LocalFileSystem
lfs = LocalFileSystem()
cfs = CachingFileSystem(
        fs=LocalFileSystem(),
        cache_storage="/data/gholl/cache/dummy",
        cache_check=600,
        check_files=False,
        expiry_times=False,
        same_names=False)
print(hash(lfs))
print(hash(cfs))

Gives:

592293666152812715
Traceback (most recent call last):
  File "/home/gholl/checkouts/protocode/mwe/cfs-hash.py", line 12, in <module>
    print(hash(cfs))
  File "/data/gholl/miniconda3/envs/py39/lib/python3.9/site-packages/fsspec/spec.py", line 162, in __hash__
    return int(self._fs_token, 16)
TypeError: int() can't convert non-string with explicit base

gerritholl · 2021-03-19T14:25:38Z

Although this works with a LocalFileSystem, it does not work with a CachingFileSystem, because the latter is not hashable:

It's hashable in fsspec master, but not in the latest release 0.8.7.

djhoese · 2021-03-19T14:26:31Z

It's hashable in fsspec master, but not in the latest release 0.8.7.

Sounds like a bug?

Edit: A bug that was fixed?

gerritholl · 2021-03-19T14:32:37Z

A bug that was fixed, but not very well fixed, as the hash is equal for file system instances that aren't equal…

fsspec/filesystem_spec#576

fsspec/filesystem_spec#577

ghost · 2021-03-19T14:58:51Z

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

mraspaud · 2021-03-19T20:13:46Z

could you work around the problem by using the json representation of the filesystem and hash that?

gerritholl · 2021-03-22T08:43:04Z

could you work around the problem by using the json representation of the filesystem and hash that?

Unfortunately not, because those are equal too!

In [15]: CachingFileSystem(fs=LocalFileSystem()).to_json()
Out[15]: '{"cls": "fsspec.implementations.local.LocalFileSystem", "protocol": "file", "args": []}'

Probably the same underlying bug causing the same hash is also causing the same JSON...

Add a test to ensure that FSFile objects that should hash differently actually do hash differently.

gerritholl · 2021-03-22T08:45:07Z

The remaining failing test is caused by a problem in fsspec, see fsspec/filesystem_spec#576 and fsspec/filesystem_spec#577.

Adapt FSFile hashing for fsspec#578, which disables JSON for CachingFileSystem because this representation is currently incorrect and thus not implemented.

Some types of some versions of fsspec implementations may return TypeError when attempted to be hashed. Capture this and use fallback option.

Add a workaround using pickle in FSFile.__hash__. This workaround is probably slow and I'm happy to hear better suggestions, but in currently released versions of fsspec as well as in fsspec master hashing of CachingFileSystem is broken.

Add more detail in the comment on the except-block for resorting to pickling for hashing.

gerritholl · 2021-03-22T13:04:26Z

NB, fsspec/filesystem_spec#578 is now merged, so with fsspec master this PR now works with CachedFileSystem without the terrible pickle workaround (but JSON still does not work for CachedFileSystem).

mraspaud

Thanks for implementing a future-proof hashing. Just one comment.

satpy/tests/test_scene.py

In the fsfile init test, replace the string "dummy" by a long random string that is very unlikely to exist as a file.

Make FSFile hashable again

a217f35

FSFile objects had been inadvertently made unhashable by pytroll#1582 . Make FSFile objects hashable again by implementing a __hash__ method. - [x] Fixes pytroll#1604 - [x] Fixes pytroll#1605

gerritholl requested review from djhoese and mraspaud as code owners March 19, 2021 13:54

gerritholl added 2 commits March 19, 2021 14:57

PEP 257 fixes.

011cd24

Remove blank line after docstring to meet PEP 257 requirements as requested by CI.

More precise ValueError match + cosmetic fix

25390bc

In test_init_with_fsfile, when capturing the ValueError for "no supported files found", more precisely capture the error message Satpy reports in this situation. Also remove a spurious # in the middle of a comment line.

Test that different FSfiles hash differently

b5deaad

Add a test to ensure that FSFile objects that should hash differently actually do hash differently.

gerritholl added 4 commits March 22, 2021 11:16

Adapt FSFile hashing for fsspec#578

4339460

Adapt FSFile hashing for fsspec#578, which disables JSON for CachingFileSystem because this representation is currently incorrect and thus not implemented.

Adapt for TypeError raised by hash(fs)

d61c16b

Some types of some versions of fsspec implementations may return TypeError when attempted to be hashed. Capture this and use fallback option.

Add workaround for failing fsspec hash

ec2847b

Add a workaround using pickle in FSFile.__hash__. This workaround is probably slow and I'm happy to hear better suggestions, but in currently released versions of fsspec as well as in fsspec master hashing of CachingFileSystem is broken.

more detail in comment

55c09b9

Add more detail in the comment on the except-block for resorting to pickling for hashing.

mraspaud assigned gerritholl Mar 22, 2021

mraspaud added the bug label Mar 22, 2021

mraspaud reviewed Mar 22, 2021

View reviewed changes

satpy/tests/test_scene.py Outdated Show resolved Hide resolved

In fsfile test, replace dummy by random string.

a03208d

In the fsfile init test, replace the string "dummy" by a long random string that is very unlikely to exist as a file.

mraspaud merged commit f6ed5d6 into pytroll:master Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make FSFile hashable again #1606

Make FSFile hashable again #1606

gerritholl commented Mar 19, 2021

gerritholl commented Mar 19, 2021

djhoese commented Mar 19, 2021

codecov bot commented Mar 19, 2021 •

edited

Loading

gerritholl commented Mar 19, 2021

gerritholl commented Mar 19, 2021

djhoese commented Mar 19, 2021 •

edited

Loading

gerritholl commented Mar 19, 2021

ghost commented Mar 19, 2021

mraspaud commented Mar 19, 2021

gerritholl commented Mar 22, 2021

gerritholl commented Mar 22, 2021

gerritholl commented Mar 22, 2021

mraspaud left a comment

Make FSFile hashable again #1606

Make FSFile hashable again #1606

Conversation

gerritholl commented Mar 19, 2021

gerritholl commented Mar 19, 2021

djhoese commented Mar 19, 2021

codecov bot commented Mar 19, 2021 • edited Loading

Codecov Report

gerritholl commented Mar 19, 2021

gerritholl commented Mar 19, 2021

djhoese commented Mar 19, 2021 • edited Loading

gerritholl commented Mar 19, 2021

ghost commented Mar 19, 2021

DeepCode failed to analyze this pull request

mraspaud commented Mar 19, 2021

gerritholl commented Mar 22, 2021

gerritholl commented Mar 22, 2021

gerritholl commented Mar 22, 2021

mraspaud left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 19, 2021 •

edited

Loading

djhoese commented Mar 19, 2021 •

edited

Loading