Add support for s3 buckets in OLCI and ABI l1 readers #1439

mraspaud · 2020-11-16T09:34:07Z

This PR allows s3 file objects to be passed to the olci readers. To do this, is implements a new class called FSFile that abides to the PathLike protocol.

Tests added
Passes flake8 satpy
Fully documented

codecov · 2020-11-16T09:56:52Z

Codecov Report

Merging #1439 (d7d227b) into master (d8d5e9e) will decrease coverage by 0.16%.
The diff coverage is 97.97%.

@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
- Coverage   90.74%   90.58%   -0.17%     
==========================================
  Files         238      239       +1     
  Lines       34139    34313     +174     
==========================================
+ Hits        30980    31081     +101     
- Misses       3159     3232      +73

Flag	Coverage Δ
behaviourtests	`4.53% <5.05%> (?)`
unittests	`91.05% <97.97%> (?)`

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

Impacted Files	Coverage Δ
satpy/readers/olci_nc.py	`91.42% <89.47%> (+1.28%)`	⬆️
satpy/readers/abi_base.py	`94.59% <94.11%> (+2.05%)`	⬆️
satpy/_compat.py	`100.00% <100.00%> (ø)`
satpy/readers/__init__.py	`97.45% <100.00%> (+0.35%)`	⬆️
satpy/readers/file_handlers.py	`97.18% <100.00%> (+1.23%)`	⬆️
satpy/tests/reader_tests/test_abi_l1b.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_olci_nc.py	`100.00% <100.00%> (ø)`
satpy/tests/test_file_handlers.py	`100.00% <100.00%> (ø)`
satpy/tests/test_readers.py	`99.19% <100.00%> (+0.14%)`	⬆️
satpy/utils.py	`24.09% <0.00%> (-46.39%)`	⬇️
... and 9 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 d8d5e9e...d7d227b. Read the comment docs.

coveralls · 2020-11-16T10:10:11Z

Coverage increased (+0.009%) to 90.755% when pulling 36ff447 on mraspaud:feature-olci-s3 into d8d5e9e on pytroll:master.

ghost · 2020-11-18T16:47:44Z

DeepCode's analysis on #d7d227 found:

ℹ️ 2 minor issues. 👇
✔️ 1 issue was fixed.

Top issues

Description	Example fixes
Unused cached_property imported from functools Occurrences: _compat.py:21	🔧 Example fixes
Value 'self.nc' is unsubscriptable Occurrences: olci_nc.py:202 olci_nc.py:225 olci_nc.py:227	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

gerritholl · 2020-11-20T14:43:50Z

Is this intended as an alternative or replacement for #1321?

mraspaud · 2020-11-20T14:50:11Z

Yes.

gerritholl · 2020-11-20T14:52:06Z

I haven't tried it yet, but it looks interesting. To be even more useful for my use case, find_files_and_readers should, when passed a file_system object via its fs argument, return instances of your FSFile class. Do you plan to implement this? If not I could look into contributing that (not sure when).

mraspaud · 2020-11-20T14:55:14Z

Yes, that is the plan indeed, for an upcoming PR. I wanted to keep this PR at a reasonable size. We'll see who gets there first.

I also have a idea for an enhancement to find_files_and_reader that, when passed an archive and not finding a matching reader, will put an fsspec wrapper around the archive to be able to look for files inside it for a matching reader.

mraspaud · 2020-11-20T14:57:37Z

Just a note though that this PR just fixes the olci readers to accept FSFiles. The other will for now probably not work with it.

mraspaud · 2020-11-20T14:58:39Z

@carloshorn you might want to have a look at this PR too.

gerritholl

This looks interesting, good work. I'm looking forward to trying it out. Just a few questions for clarification (see comments for details).

gerritholl · 2020-11-20T15:04:55Z

satpy/readers/__init__.py

+
+@total_ordering
+class FSFile(os.PathLike):
+    """Implementation of a PathLike file object, that can be opened.


I'm confused by this line. Is it a file object or a path object? If it's a file object it's already opened? I think os.PathLike represents a path, not an open file.

It is a path indeed, but can be opened to return a file object.

gerritholl · 2020-11-20T15:05:19Z

satpy/readers/__init__.py

+
+    This is made to be use in conjuction with fsspec or s3fs. For example::
+
+        zipfile = S3B_OL_2_WFR____20201103T100807_20201103T101107_20201103T121330_0179_045_179_1980_MAR_O_NR_002.zip


There appear to be some string-delimiting quotes missing here.

indeed thanks.

gerritholl · 2020-11-20T15:05:43Z

satpy/readers/__init__.py

+    This is made to be use in conjuction with fsspec or s3fs. For example::
+
+        zipfile = S3B_OL_2_WFR____20201103T100807_20201103T101107_20201103T121330_0179_045_179_1980_MAR_O_NR_002.zip
+        filename = "sentinel-s3-ol2wfr-zips/2020/11/03/" + filename


Should this be + zipfile?

gerritholl · 2020-11-20T15:06:45Z

satpy/readers/__init__.py

+
+        zipfile = S3B_OL_2_WFR____20201103T100807_20201103T101107_20201103T121330_0179_045_179_1980_MAR_O_NR_002.zip
+        filename = "sentinel-s3-ol2wfr-zips/2020/11/03/" + filename
+        the_files = fsspec.open_files("simplecache::zip://**/*.nc::s3://" + filename, s3={'anon': False})


You're passing 'anon': False — might you have an example that is available with anonymous access such that any user can easily try out the example?

I don't have open an example for olci files unfortunately, and this is the only reader supporting this right now.

Fixed through switching to an abi example.

gerritholl · 2020-11-20T15:15:34Z

satpy/readers/olci_nc.py

+    @property
+    @lru_cache(maxsize=2)
+    def nc(self):


Mostly for my curiosity: I've never seen lru_cache used on a property before. I don't know much about OLCI. What is the purpose and effect of a cache of size 2 on this property? Does that mean that if I open 3 OLCI files that the one I opened first is closed again? Or just that there is work redone if .nc is accessed on the first one?

I put size 2 for safety, but in principle only 1 is needed, as this is a property on the instance. So each instance of the file handler will have a different cache for this accessor, and allows repeated calls to the property not to trigger a reread (especially important when this is a remote file of course).

Wasn't this cached_property before? What's the difference?

it was, but cached_property was introduced in python 3.8, so I have to pass on that for now!

Ah ok. What about a try/except ImportError on that? Can lru_cache take a maxsize=1? Does it default to that? Basically can we use cached_property when it exists, but use lru_cache for backwards compatibility for now?

I can do that.

Hmmm, I thought this would do it, but no... Will investigate more tomorrow

def cached_property(func): return property(lru_cache(maxsize=1)(func))

ok, got it to work now.

gerritholl · 2020-11-20T15:21:08Z

satpy/tests/test_readers.py

+        with suppress(PermissionError):
+            os.remove(self.zip_name)


How can this lead to PermissionError when you just created this file in setUp?

Ask windows...

carloshorn · 2020-11-23T11:07:27Z

@carloshorn you might want to have a look at this PR too.

@mraspaud thanks for adding it. Good idea to pass the file system with the file path object. This preserves the satpy interface and avoids passing the file system down the road as in my draft. I am already convinced of using fsspec filesystems, because it gives the developers a hook where they can reconstruct any expected directory structure and filename conventions without forcing any changes/copies/downloads on the physical filesystem.
Still, I do not like that many satpy readers extract metadata from filenames and therefore set constraints on them, but at least this PR gives a solution on how to deal with it programmatically.

If this PR gets merged, I would volunteer to add the FSFile support for the avhrr _l1b_gaclac reader.

mraspaud · 2020-11-23T12:14:21Z

@carloshorn you might want to have a look at this PR too.

@mraspaud thanks for adding it. Good idea to pass the file system with the file path object. This preserves the satpy interface and avoids passing the file system down the road as in my draft. I am already convinced of using fsspec filesystems, because it gives the developers a hook where they can reconstruct any expected directory structure and filename conventions without forcing any changes/copies/downloads on the physical filesystem.

👍

Still, I do not like that many satpy readers extract metadata from filenames and therefore set constraints on them, but at least this PR gives a solution on how to deal with it programmatically.

Interesting, we should discuss this next week (PCW) ?

If this PR gets merged, I would volunteer to add the FSFile support for the avhrr _l1b_gaclac reader.

Sounds great!

It was introduced in python 3.8, so we have to wait a bit for it.

mraspaud · 2020-12-01T13:40:11Z

This is ready for re-review and merging I think

sfinkens · 2020-12-01T15:59:44Z

I don't have much expertise here, but to me it looks very thin and simple 👍 Far from re-inventing the wheel in my opinion.

djhoese

Do you want to re-title this since it includes changes to ABI? Maybe "Add experimental S3 support to OLCI and ABI readers"?

Do you want to do the self.nc property trick to the abi base reader too?

mraspaud · 2020-12-01T16:22:33Z

Will do (both)

mraspaud · 2020-12-02T09:15:03Z

Done

djhoese

One more small/optional request and one question. Otherwise LGTM.

djhoese · 2020-12-02T11:59:15Z

satpy/readers/olci_nc.py

+
+    def cached_property(func):
+        """Port back functools.cached_property."""
+        return property(lru_cache(maxsize=None)(func))


So now you're going to hate me. What about moving this into it's own _compat.py module so I can use it in other readers and it can be shared by the readers here?

I can do that. And no I don't hate you :)

djhoese · 2020-12-02T12:00:39Z

satpy/readers/file_handlers.py

-            self.filename = str(filename)
-        else:
-            self.filename = filename
+        self.filename = filename


This change will break pathlib objects for all other readers right? (assuming the low-level I/O library doesn't support them)

That said, I'm ok with this.

probably yes... Is this bad?

I think @gerritholl was the first user to point out that Satpy didn't work with pathlib objects so he should maybe make the final decision. It probably isn't great that this breaks it for all other readers, but I'm not sure how many readers need strings for their lower-level I/O libraries either.

djhoese · 2020-12-02T12:45:49Z

satpy/_compat.py

+
+def cached_property(func):
+    """Port back functools.cached_property."""
+    return property(lru_cache(maxsize=None)(func))


Ah I was thinking putting the import of cached_property in here too. That way I could do from satpy._compat import cached_property with no try/except in my reader module. Thoughts?

you got it.

djhoese

Great! Merge when you can and I'll use this cached_property compat decorator in my GAASP reader.

Allow path object to pass through to the file handler

0ef06c0

mraspaud self-assigned this Nov 16, 2020

mraspaud added component:readers enhancement code enhancements, features, improvements labels Nov 16, 2020

mraspaud added 2 commits November 16, 2020 11:43

Merge branch 'master' into feature-olci-s3

36ff447

Add tests

47a4dcf

Add FSFile

973ca18

mraspaud requested review from djhoese and gerritholl November 20, 2020 13:06

mraspaud marked this pull request as ready for review November 20, 2020 13:06

mraspaud added 2 commits November 20, 2020 14:12

Make deepcode happier

f04c137

Fix windows tests

dedeaea

Fix windows tests

8afdb38

gerritholl reviewed Nov 20, 2020

View reviewed changes

mraspaud added 5 commits November 23, 2020 15:11

Add support for FSFile in abi l1b reader

445fe1b

Refactor olci file opening

02ff1f8

Change FSFile example in class docstring

407fa63

Fix style

719386f

Refactor ABI base

9c5b55f

Clean up style

034b521

gerritholl mentioned this pull request Nov 30, 2020

Adapt method for remote reading gerritholl/sattools#37

Open

mraspaud added 7 commits December 1, 2020 13:34

Replace cached_property with lru_cache

b2956f2

It was introduced in python 3.8, so we have to wait a bit for it.

Mute quality note in tests

2b3f026

Mute quality note in tests

4bffac3

Mute quality note in tests

eb7b9c2

Fix typo

4dd8b20

Fix lru_cache call for python 3.7

ecdcbce

Fix typo

be6c757

djhoese reviewed Dec 1, 2020

View reviewed changes

mraspaud changed the title ~~Add support for s3 buckets in olci reader~~ Add support for s3 buckets in OLCI and ABI l1 readers Dec 2, 2020

Fix cached_property in ABI readers

4e3b7b5

djhoese reviewed Dec 2, 2020

View reviewed changes

Factorize cached_property backport

81505c6

djhoese reviewed Dec 2, 2020

View reviewed changes

Put try/except for cached_property backport in _compat

d7d227b

djhoese approved these changes Dec 2, 2020

View reviewed changes

mraspaud merged commit c0e21e6 into pytroll:master Dec 2, 2020

gerritholl mentioned this pull request Dec 4, 2020

Allow find_files_and_readers to return FSFile instances #1471

Open

This was referenced Dec 11, 2020

Allow PathLike objects pytroll/pygac#92

Merged

fsspec FileSystem support #1321

Closed

martin-rdz mentioned this pull request Jul 23, 2023

Add fsspec functionality to viirs_sdr reader #2534

Merged

3 tasks


		This is made to be use in conjuction with fsspec or s3fs. For example::

		zipfile = S3B_OL_2_WFR____20201103T100807_20201103T101107_20201103T121330_0179_045_179_1980_MAR_O_NR_002.zip

Add support for s3 buckets in OLCI and ABI l1 readers #1439

Add support for s3 buckets in OLCI and ABI l1 readers #1439

Conversation

mraspaud commented Nov 16, 2020 • edited Loading

codecov bot commented Nov 16, 2020 • edited Loading

Codecov Report

coveralls commented Nov 16, 2020 • edited Loading

ghost commented Nov 18, 2020 • edited by ghost Loading

DeepCode's analysis on #d7d227 found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

gerritholl commented Nov 20, 2020

mraspaud commented Nov 20, 2020

gerritholl commented Nov 20, 2020

mraspaud commented Nov 20, 2020

mraspaud commented Nov 20, 2020

mraspaud commented Nov 20, 2020

gerritholl left a 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

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

carloshorn commented Nov 23, 2020

mraspaud commented Nov 23, 2020

mraspaud commented Dec 1, 2020

sfinkens commented Dec 1, 2020

djhoese left a comment

Choose a reason for hiding this comment

mraspaud commented Dec 1, 2020

mraspaud commented Dec 2, 2020

djhoese left a 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

djhoese left a comment

Choose a reason for hiding this comment

mraspaud commented Nov 16, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading

coveralls commented Nov 16, 2020 •

edited

Loading

ghost commented Nov 18, 2020 •

edited by ghost

Loading