Add support for fsspec files to seviri_l1b_nc reader #1547

BENR0 · 2021-02-17T16:24:32Z

The seviri netcdf reader now supports opening files from fsspec.open_file objects together with FSFile.

FSFile is still pretty new but I think this is a pretty interesting thing for users especially if this eventually will be implemented by more and more readers. So I am wondering if this should be documented more visibly than in the docstrings of FSFile, possibly with a list of readers supporting this?

Tests added

mraspaud

Great work, thanks for implementing the FSFile support in the nc seviri reader! I only have a couple of suggestions inline.

mraspaud · 2021-02-17T16:38:56Z

satpy/readers/seviri_l1b_nc.py

+        f_obj = open_file_or_filename(self.filename)

-            self.nc = xr.open_dataset(self.filename,
-                                      decode_cf=True,
-                                      mask_and_scale=False,
-                                      chunks=CHUNK_SIZE)
+        dataset = xr.open_dataset(f_obj,
+                                  decode_cf=True,
+                                  mask_and_scale=False,
+                                  chunks=CHUNK_SIZE)


This pattern is being used in multiple places now, and it might need to evolve in the future if fsspec starts return PathLib compliant files or if xarray starts supporting s3buckets for example. I think it would be beneficial if we could create a reader agnostic function that would do this, so in the future if the APIs of dependencies change, it would make it easier to change. So how about a function along these lines:

def open_dataset(filename, *args, **kwargs): f_obj = open_file_or_filename(filename) return xr.open_dataset(f_obj, *args, **kwargs)

This goes a bit against my conviction that we should avoid *args and **kwargs though... maybe a decorator would be an alternative?

Do I understand correctly that you want to move the logic into a function completely outside of the reader so it can be used in other readers and the nc method just calls open_dataset?

Yes exactly.

Ok. So where would you think that function should go (maybe in file_handlers.py)?

Sounds about right!

mraspaud · 2021-02-17T16:40:49Z

satpy/readers/seviri_l1b_nc.py

+            equatorial_radius = equatorial_radius[0]
+            polar_radius = polar_radius[0]
+            ssp_lon = ssp_lon[0]
+            self.mda['vis_ir_column_dir_grid_step'] = self.mda['vis_ir_column_dir_grid_step'][0]
+            self.mda['vis_ir_line_dir_grid_step'] = self.mda['vis_ir_line_dir_grid_step'][0]


I think item would suit better here. Also maybe this can be delegated to a small method (including the if statement)

Suggested change

equatorial_radius = equatorial_radius[0]

polar_radius = polar_radius[0]

ssp_lon = ssp_lon[0]

self.mda['vis_ir_column_dir_grid_step'] = self.mda['vis_ir_column_dir_grid_step'][0]

self.mda['vis_ir_line_dir_grid_step'] = self.mda['vis_ir_line_dir_grid_step'][0]

equatorial_radius = equatorial_radius[0]

polar_radius = polar_radius.item()

ssp_lon = ssp_lon.item()

self.mda['vis_ir_column_dir_grid_step'] = self.mda['vis_ir_column_dir_grid_step'].item()

self.mda['vis_ir_line_dir_grid_step'] = self.mda['vis_ir_line_dir_grid_step'].item()

Will refactor to item. I was thinking about a method too but I am not sure if it is worth the overhead?

I wouldn't bother to put item in a special method.

satpy/readers/seviri_l1b_nc.py

mraspaud · 2021-02-17T16:43:25Z

satpy/tests/reader_tests/test_seviri_l1b_nc.py

@@ -32,7 +32,7 @@
 from satpy.tests.utils import make_dataid


-def new_read_file(instance):
+def new_read_file(h5):


h5 isn't really an explicit name, could you rename this parameter?

mraspaud · 2021-02-17T16:48:21Z

Indeed more documentation on FSFile would be nice. I think it deserves at least a paragraph in the main documentation, with an example or two.

As for saying which readers support this, I would say we could probably use the main reader table for that (together with 'status' maybe?)

BENR0 · 2021-02-17T18:16:48Z

Yes I think that is a good idea to include it in the main reader table and additionally add some examples.

ghost · 2021-02-18T13:41:27Z

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

Wow, thanks for all the new documentation! Some comment inline.

mraspaud · 2021-02-25T08:03:38Z

doc/source/dev_guide/remote_file_support.rst

+for supported readers to read from remote filesystems has been added.
+
+To add this feature to a reader the call to :func:`xarray.open_dataset`
+hast to be replaced by the function :func:`~satpy.readers.file_handlers.open_dataset`


Suggested change

hast to be replaced by the function :func:`~satpy.readers.file_handlers.open_dataset`

has to be replaced by the function :func:`~satpy.readers.file_handlers.open_dataset`

mraspaud · 2021-02-25T08:09:32Z

satpy/readers/seviri_l1b_nc.py

+        # if FSFile is used h5netcdf engine is used which outputs arrays instead of floats for attributes
+        if isinstance(equatorial_radius, np.ndarray):
+            equatorial_radius = equatorial_radius.item()
+            polar_radius = polar_radius.item()
+            ssp_lon = ssp_lon.item()
+            self.mda['vis_ir_column_dir_grid_step'] = self.mda['vis_ir_column_dir_grid_step'].item()
+            self.mda['vis_ir_line_dir_grid_step'] = self.mda['vis_ir_line_dir_grid_step'].item()


Could you make this a separate method, to shorten get_metadata?

mraspaud · 2021-02-25T08:12:42Z

satpy/readers/file_handlers.py

+def open_dataset(filename, *args, **kwargs):
+    """Open a file with xarray.
+
+    Args:
+       filename (Union[str, FSFile]):
+           The path to the file to open. Can be a `string` or
+           :class:`~satpy.readers.FSFile` object which allows using
+           `fsspec` or `s3fs` like files.
+
+    Returns:
+       xarray.Dataset:
+
+    Notes:
+       This can be used to enable readers to open remote files.
+    """
+    f_obj = open_file_or_filename(filename)
+    return xr.open_dataset(f_obj, *args, **kwargs)


Could you add a small test for this?

I think this refactor probably is the reason why some tests in the seviri nc reader are failing now too. I will have a look at it and will try to fix it.

@BENR0 did you have some time to look at this?

Sorry I didn't have time to look at it yet due to a project deadline. Maybe I have a chance later today otherwise I will look at it during the weekend.

no problem, take it easy :)

codecov · 2021-03-09T13:18:50Z

Codecov Report

Merging #1547 (dbe2173) into main (c181288) will increase coverage by 0.00%.
The diff coverage is 89.09%.

@@           Coverage Diff           @@
##             main    #1547   +/-   ##
=======================================
  Coverage   93.87%   93.88%           
=======================================
  Files         283      283           
  Lines       43046    43073   +27     
=======================================
+ Hits        40409    40437   +28     
+ Misses       2637     2636    -1

Flag	Coverage Δ
behaviourtests	`4.78% <5.45%> (+<0.01%)`	⬆️
unittests	`94.53% <89.09%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/readers/seviri_l1b_nc.py	`68.51% <77.77%> (+1.22%)`	⬆️
satpy/readers/file_handlers.py	`97.80% <100.00%> (+0.12%)`	⬆️
satpy/tests/reader_tests/test_seviri_l1b_nc.py	`100.00% <100.00%> (ø)`
satpy/tests/test_file_handlers.py	`100.00% <100.00%> (ø)`

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 c181288...dbe2173. Read the comment docs.

mraspaud · 2021-03-10T08:42:50Z

satpy/tests/test_file_handlers.py

+
+def test_open_dataset():
+    """Test xr.open_dataset wrapper."""
+    fn = "path/to/file.nc"


Should this be a MagicMock? that would allow to check if open was called on it.

I had this mocked at first but then I thought that this probably was covered by the test of the open_file_or_filename function. I admit I didn't check but I have seen now that the function is not covered by a test. Should I mock it and test it here or should that be a separate test?

well, what we want test here is that the dataset is opened whether it's a filename or a fsfile object, right? so I think both scenarios should be tested :)

Yes the dataset should be opened regardless. But what I meant was; the logic for this is covered in open_file_or_filename and not here. That is why I assumed it should not necessarily be tested here. Anyway probably that is nitpicking as long as it is covered. I will add it here then.

coveralls · 2021-12-07T14:39:19Z

Coverage increased (+0.006%) to 94.475% when pulling dbe2173 on BENR0:feat_fsfile_support_seviri_nc into c181288 on pytroll:main.

BENR0 · 2022-05-11T13:42:26Z

Ok somehow there went something wrong during the rebase I think. Need to check that out. But in general the changes in this PR should be ok to be merged.

gerritholl · 2022-05-12T13:56:42Z

Ok somehow there went something wrong during the rebase I think. Need to check that out. But in general the changes in this PR should be ok to be merged.

Does this help? https://stackoverflow.com/questions/60003798/why-does-a-pull-request-show-extra-commits-after-a-rebase

BENR0 · 2022-05-24T07:26:58Z

@djhoese @mraspaud @sfinkens: from my side this is also ready to be merged. Anything from your point of view that still needs to be done?

mraspaud

LGTM

sfinkens · 2022-07-25T10:54:02Z

Nice work, thanks!

BENR0 requested review from djhoese, mraspaud and sfinkens as code owners February 17, 2021 16:24

mraspaud reviewed Feb 17, 2021

View reviewed changes

BENR0 force-pushed the feat_fsfile_support_seviri_nc branch from 30f5cd0 to ed77e35 Compare February 25, 2021 07:38

mraspaud assigned BENR0 Feb 25, 2021

mraspaud added component:readers enhancement code enhancements, features, improvements labels Feb 25, 2021

mraspaud reviewed Feb 25, 2021

View reviewed changes

mraspaud added this to the v0.26.0 milestone Feb 25, 2021

BENR0 requested a review from adybbroe as a code owner March 9, 2021 13:07

mraspaud reviewed Mar 10, 2021

View reviewed changes

mraspaud removed this from the v0.26.0 milestone Mar 15, 2021

BENR0 added 3 commits December 7, 2021 14:44

feat: add fsspec to reader

b59e27a

Add tests

dbccbc4

doc: add fsspec documentation

a09a4be

BENR0 force-pushed the feat_fsfile_support_seviri_nc branch from 195f3d1 to a09a4be Compare December 7, 2021 14:10

pnuu mentioned this pull request May 5, 2022

Convert remote files to FSFile objects automatically #2096

Merged

2 tasks

BENR0 requested review from pnuu and gerritholl as code owners May 11, 2022 13:29

BENR0 added 2 commits May 13, 2022 12:03

Add: status and fsspec support info to yaml file

6ab876d

Merge main

dbe2173

BENR0 force-pushed the feat_fsfile_support_seviri_nc branch from 7ce15bd to dbe2173 Compare May 13, 2022 10:20

mraspaud approved these changes May 24, 2022

View reviewed changes

mraspaud merged commit cd057ca into pytroll:main May 24, 2022

BENR0 deleted the feat_fsfile_support_seviri_nc branch May 25, 2022 06:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for fsspec files to seviri_l1b_nc reader #1547

Add support for fsspec files to seviri_l1b_nc reader #1547

BENR0 commented Feb 17, 2021

mraspaud left a comment

mraspaud Feb 17, 2021

BENR0 Feb 17, 2021

mraspaud Feb 17, 2021

BENR0 Feb 17, 2021

mraspaud Feb 17, 2021

mraspaud Feb 17, 2021

BENR0 Feb 17, 2021

mraspaud Feb 17, 2021

mraspaud Feb 17, 2021

BENR0 Feb 17, 2021

mraspaud commented Feb 17, 2021

BENR0 commented Feb 17, 2021

ghost commented Feb 18, 2021

mraspaud left a comment

mraspaud Feb 25, 2021

mraspaud Feb 25, 2021

mraspaud Feb 25, 2021

BENR0 Feb 25, 2021

mraspaud Mar 4, 2021

BENR0 Mar 5, 2021

mraspaud Mar 5, 2021

codecov bot commented Mar 9, 2021 •

edited

Loading

mraspaud Mar 10, 2021

BENR0 Mar 10, 2021

mraspaud Mar 10, 2021

BENR0 Mar 10, 2021

coveralls commented Dec 7, 2021 •

edited

Loading

BENR0 commented May 11, 2022

gerritholl commented May 12, 2022

BENR0 commented May 24, 2022

mraspaud left a comment

sfinkens commented Jul 25, 2022

	hast to be replaced by the function :func:`~satpy.readers.file_handlers.open_dataset`
	has to be replaced by the function :func:`~satpy.readers.file_handlers.open_dataset`

Add support for fsspec files to seviri_l1b_nc reader #1547

Add support for fsspec files to seviri_l1b_nc reader #1547

Conversation

BENR0 commented Feb 17, 2021

mraspaud 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

mraspaud commented Feb 17, 2021

BENR0 commented Feb 17, 2021

ghost commented Feb 18, 2021

DeepCode failed to analyze this pull request

mraspaud 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

codecov bot commented Mar 9, 2021 • edited Loading

Codecov Report

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

coveralls commented Dec 7, 2021 • edited Loading

BENR0 commented May 11, 2022

gerritholl commented May 12, 2022

BENR0 commented May 24, 2022

mraspaud left a comment

Choose a reason for hiding this comment

sfinkens commented Jul 25, 2022

codecov bot commented Mar 9, 2021 •

edited

Loading

coveralls commented Dec 7, 2021 •

edited

Loading