Add process_input and process_chunk hooks #65

davidbrochart · 2021-01-28T14:10:10Z

This PR adds a process_input parameter to NetCDFtoZarrSequentialRecipe, which is a function optionally called after each input has been opened. It takes in the name of the opened file and the dataset, and returns a dataset.
The motivation is that it is very likely that we need to tweak the input data for the rest of the flow. For instance, some datasets have their timestamp encoded in the file name, so if you want to concatenate on a time dimension, you need to extract the timestamp from the file name and put it in a time coordinate.
If you think this can go in, I'll write a test.

martindurant · 2021-01-28T14:19:40Z

This kind of functionality would be nice to have in Intake catalogues in general, and then you could express your inputs as usual Data Sources. We have that, e.g., for the CSV case (intake.source.base.PatternMixin), so the template syntax used there might be a good model to adhere to.

rabernat · 2021-01-28T14:32:49Z

Thanks David! In general this is great and is and partially addresses #48. If you can also add a process_chunk option, then it will close #48 entirely.

We will need tests. I would prefer to have these things parametrized such that we are testing both with and without the option. I was puzzled why this didn't trigger codecov (see #61), but I realized I had not activated the webhook. So if you push again, we should get a coverage report.

rabernat · 2021-01-28T14:34:50Z

so the template syntax used there might be a good model to adhere to.

@martindurant could you link us directly to the code you're talking about?

martindurant · 2021-01-28T14:40:48Z

The docstring for the CSV case: https://intake.readthedocs.io/en/latest/api_user.html#intake.source.csv.CSVSource.__init__
(path_as_pattern kwargs)
A blog about the functionality: https://www.anaconda.com/blog/intake-parsing-data-from-filenames-and-paths
Implementation: https://github.com/intake/intake/blob/master/intake/source/base.py#L430

davidbrochart · 2021-01-28T14:44:35Z

Awesome, thanks @martindurant and @rabernat.

rabernat · 2021-01-28T14:46:16Z

See #63 (comment) for a discussion of how we imagine refactoring this monster class.

codecov-io · 2021-01-28T18:49:10Z

Codecov Report

Merging #65 (5ec76b4) into master (266318d) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   91.07%   91.24%   +0.16%     
==========================================
  Files           4        4              
  Lines         269      274       +5     
  Branches       20       22       +2     
==========================================
+ Hits          245      250       +5     
  Misses         21       21              
  Partials        3        3

Impacted Files	Coverage Δ
pangeo_forge/recipe.py	`90.60% <100.00%> (+0.32%)`	⬆️
pangeo_forge/utils.py	`100.00% <0.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 46b465c...5ec76b4. Read the comment docs.

davidbrochart · 2021-01-28T18:49:17Z

I think the pattern matching in the file name mentioned by Martin could be implemented in another PR. We could provide decorators that could be applied to the hooks, like this:

@match(fname="data_{year}_{month}_{day}.nc")
def process_input(ds):
    # do some processing on ds using year, month and day
    return ds

What do you think?

rabernat

🎉 This is fantastic @davidbrochart!

rabernat · 2021-01-28T18:58:03Z

tests/test_recipe.py

@@ -8,6 +8,13 @@
 dummy_fnames = ["a.nc", "b.nc", "c.nc"]


+def incr_date(ds, fname=""):


Can you explain the point of the fname kwarg?

We use incr_date for both process_input and process_chunk in the test, but they have a different signature:

process_input(ds, fname) -> ds

process_chunk(ds) -> ds

Would you like to have different hooks?

Ok, yes I see now. Adding the fname argument in the process_input makes sense I guess. If we were opening files directly with xarray (rather than first with fsspec) this information would also be available in the xarray .encoding attributes. But that's not the case here.

I was also confused on this point. Can we rename fname to filename? I read fname as function name.

rabernat · 2021-01-28T18:58:29Z

tests/test_recipe.py

+        # check that the process_chunk hook made some changes
+        assert not ds_target.identical(ds_expected)
+        # apply these changes to the expected dataset
+        ds_expected = process_chunk(ds_expected)


Great test.

rabernat · 2021-01-28T19:13:12Z

It would be nice to get #6 in and then rebase so we could see the API documentation built from this PR.

davidbrochart · 2021-01-28T19:32:26Z

I think you meant #46, I'm looking at it.

rabernat · 2021-01-28T19:50:32Z

Docs look great! Thanks so much for your work on this David!

I'd like to get one more set of eyes on this. I've tagged a few potential reviewers.

pangeo_forge/recipe.py

TomAugspurger · 2021-01-28T21:38:58Z

tests/test_recipe.py

@@ -8,6 +8,13 @@
 dummy_fnames = ["a.nc", "b.nc", "c.nc"]


+def incr_date(ds, fname=""):


I was also confused on this point. Can we rename fname to filename? I read fname as function name.

Co-authored-by: Tom Augspurger <1312546+TomAugspurger@users.noreply.github.com>

TomAugspurger · 2021-01-28T22:26:50Z

pangeo_forge/recipe.py

@@ -156,6 +160,8 @@ class NetCDFtoZarrSequentialRecipe(BaseRecipe):
    xarray_concat_kwargs: dict = field(default_factory=dict)
    delete_input_encoding: bool = True
    fsspec_open_kwargs: dict = field(default_factory=dict)
+    process_input: Optional[Callable[[xr.Dataset, str], xr.Dataset] = None


Whoops, sorry my suggestion was missing a ].

Might want to make sure typing.Callable is imported as well.

Thanks @TomAugspurger!

pangeo_forge/recipe.py

davidbrochart · 2021-01-29T08:12:44Z

I see we're still having an issue with the HTTP server in the CI?

rabernat · 2021-01-29T12:47:17Z

I see we're still having an issue with the HTTP server in the CI?

Yes. The short term fix is maybe to just bump the sleep call again. The long term one is maybe to use this: https://pypi.org/project/pytest-httpserver/

davidbrochart · 2021-01-29T13:43:50Z

Not sure pytest-httpserver will help since the issue doesn't seem to be with pytest but with the CI (I've never seen the tests hang locally).

martindurant · 2021-01-29T13:46:25Z

I commonly have something like this to wait for servers:

    timeout = 5
    while True:
        try:
            r = requests.get(endpoint_uri)
            if r.ok:
                break
        except:
            pass
        timeout -= 0.1
        assert timeout > 0, "Timed out waiting for server"
        time.sleep(0.1)

davidbrochart · 2021-01-29T13:50:33Z

Thanks Martin, that's much better than waiting for an arbitrary amount of time! I'll open a PR.

rabernat · 2021-01-29T14:02:31Z

(I've never seen the tests hang locally).

I have! On my os-x laptop.

rabernat linked an issue Jan 28, 2021 that may be closed by this pull request

Input / chunk preprocessing #48

Closed

davidbrochart force-pushed the process_input branch from 7de9783 to 45f15b3 Compare January 28, 2021 18:51

rabernat reviewed Jan 28, 2021

View reviewed changes

davidbrochart changed the title ~~[WIP] Add process_input parameter~~ Add process_input parameter Jan 28, 2021

davidbrochart changed the title ~~Add process_input parameter~~ Add process_input and process_chunk hooks Jan 28, 2021

davidbrochart added 2 commits January 28, 2021 20:44

Add process_input parameter

55a0a57

Add process_chunk hook, test

85103f7

davidbrochart force-pushed the process_input branch from 45f15b3 to 85103f7 Compare January 28, 2021 19:44

rabernat requested review from jhamman and TomAugspurger January 28, 2021 19:50

TomAugspurger approved these changes Jan 28, 2021

View reviewed changes

Specify callable signature

a42862d

Co-authored-by: Tom Augspurger <1312546+TomAugspurger@users.noreply.github.com>

TomAugspurger reviewed Jan 28, 2021

View reviewed changes

Changes according to review

bfa6a95

davidbrochart force-pushed the process_input branch from 080c4fb to bfa6a95 Compare January 28, 2021 22:29

rabernat reviewed Jan 28, 2021

View reviewed changes

pangeo_forge/recipe.py Outdated Show resolved Hide resolved

Update pangeo_forge/recipe.py

5ec76b4

rabernat merged commit aa5f8a9 into pangeo-forge:master Jan 28, 2021

This was referenced Jan 29, 2021

Try and fix HTTP server in tests #66

Closed

Use pytest-httpserver #67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add process_input and process_chunk hooks #65

Add process_input and process_chunk hooks #65

davidbrochart commented Jan 28, 2021

martindurant commented Jan 28, 2021

rabernat commented Jan 28, 2021

rabernat commented Jan 28, 2021

martindurant commented Jan 28, 2021

davidbrochart commented Jan 28, 2021

rabernat commented Jan 28, 2021

codecov-io commented Jan 28, 2021 •

edited

Loading

davidbrochart commented Jan 28, 2021

rabernat left a comment

rabernat Jan 28, 2021

davidbrochart Jan 28, 2021

rabernat Jan 28, 2021

TomAugspurger Jan 28, 2021

rabernat Jan 28, 2021

rabernat commented Jan 28, 2021 •

edited

Loading

davidbrochart commented Jan 28, 2021

rabernat commented Jan 28, 2021

TomAugspurger Jan 28, 2021

TomAugspurger Jan 28, 2021 •

edited

Loading

davidbrochart Jan 28, 2021

davidbrochart commented Jan 29, 2021

rabernat commented Jan 29, 2021

davidbrochart commented Jan 29, 2021

martindurant commented Jan 29, 2021

davidbrochart commented Jan 29, 2021

rabernat commented Jan 29, 2021

		@@ -8,6 +8,13 @@
		dummy_fnames = ["a.nc", "b.nc", "c.nc"]


		def incr_date(ds, fname=""):

Add process_input and process_chunk hooks #65

Add process_input and process_chunk hooks #65

Conversation

davidbrochart commented Jan 28, 2021

martindurant commented Jan 28, 2021

rabernat commented Jan 28, 2021

rabernat commented Jan 28, 2021

martindurant commented Jan 28, 2021

davidbrochart commented Jan 28, 2021

rabernat commented Jan 28, 2021

codecov-io commented Jan 28, 2021 • edited Loading

Codecov Report

davidbrochart commented Jan 28, 2021

rabernat left a comment

Choose a reason for hiding this comment

rabernat Jan 28, 2021

Choose a reason for hiding this comment

davidbrochart Jan 28, 2021

Choose a reason for hiding this comment

rabernat Jan 28, 2021

Choose a reason for hiding this comment

TomAugspurger Jan 28, 2021

Choose a reason for hiding this comment

rabernat Jan 28, 2021

Choose a reason for hiding this comment

rabernat commented Jan 28, 2021 • edited Loading

davidbrochart commented Jan 28, 2021

rabernat commented Jan 28, 2021

TomAugspurger Jan 28, 2021

Choose a reason for hiding this comment

TomAugspurger Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

davidbrochart Jan 28, 2021

Choose a reason for hiding this comment

davidbrochart commented Jan 29, 2021

rabernat commented Jan 29, 2021

davidbrochart commented Jan 29, 2021

martindurant commented Jan 29, 2021

davidbrochart commented Jan 29, 2021

rabernat commented Jan 29, 2021

codecov-io commented Jan 28, 2021 •

edited

Loading

rabernat commented Jan 28, 2021 •

edited

Loading

TomAugspurger Jan 28, 2021 •

edited

Loading