Kerchunk WriteCombinedReference return fsstore #660

norlandrhagen · 2023-12-05T21:00:56Z

This PR is a clean-slate version of this PR: #636
It borrows heavily from @sbquinlan's excellent work in this PR. #654. Hopefully enabling that PR to proceed.

test_CombineReferences is now passing
The integration test that tries hrrr_kerchunk_concat_step is now passing and the xfail is removed.

sbquinlan · 2023-12-05T21:04:50Z

examples/feedstock/hrrr_kerchunk_concat_valid_time.py

+        # fails due to: _pickle.PicklingError: Can't pickle <function drop_unknown
+        #  at 0x290e46a70>: attribute lookup drop_unknown on __main__ failed
        mzz_kwargs=dict(preprocess=drop_unknown),


yeah I haven't had success passing references to functions into transform. It seems to fail at compilation. I don't have a workaround. Maybe @cisaacstern does?

Nope no secret solutions on my end. This definitely merits its own issue.

sbquinlan · 2023-12-05T22:31:08Z

pangeo_forge_recipes/writers.py

    elif file_ext == ".parquet":
-
        # Creates empty parquet store to be written to
        if full_target.exists(output_file_name):
            full_target.rm(output_file_name, recursive=True)
        full_target.makedir(output_file_name)

-        remote_protocol = _select_single_protocol(full_target)
-
        out = LazyReferenceMapper.create(refs_per_component, outpath, full_target.fs)


I'm not in love with this part, but I don't know enough about parquet + referencefs to do better.

Happy to update if you think of anything!

@sbquinlan I got rid of inferring the protocol in favor of explicitly passing it.

sbquinlan · 2023-12-05T23:07:30Z

pangeo_forge_recipes/transforms.py

@@ -446,6 +447,33 @@ def expand(self, references: beam.PCollection) -> beam.PCollection:
        )


+@dataclass
+class WriteReference(beam.PTransform, ZarrWriterMixin):


sbquinlan · 2023-12-05T23:14:44Z

pangeo_forge_recipes/transforms.py

+
+    store_name: str
+    concat_dims: List[str]
+    target_root: Union[str, FSSpecTarget, RequiredAtRuntimeDefault] = field(


I think you need to update the injections so that this gets updated from the config. That should fix the integration test failure

Great catch! I updated injections.py to add WriteReference. Any idea if any more is required for that?

Hmm that should have done it... not sure why it's still failing on that. You could just pass the value in WriteCombineReference as a workaround, but ideally what you have should work.

Can take a closer look tomorrow but off the cuff I wonder if the failure is because we need to bump the version of runner used in the integration tests

I bumped runner to the two latest tags. First kerchunk integration test is passing!
Getting a failure on the minio version though.

tests/test_integration.py::test_python_json_configs_identical PASSED tests/test_integration.py::test_integration[local_confpath-hrrr-kerchunk-concat-valid-time] XFAIL tests/test_integration.py::test_integration[local_confpath-hrrr-kerchunk-concat-step] PASSED tests/test_integration.py::test_integration[local_confpath-gpcp-from-gcs-dynamic-chunks] PASSED tests/test_integration.py::test_integration[local_confpath-terraclimate] XFAIL tests/test_integration.py::test_integration[local_confpath-narr-opendap] XFAIL tests/test_integration.py::test_integration[local_confpath-noaa-oisst] PASSED tests/test_integration.py::test_integration[local_confpath-gpcp-from-gcs] PASSED tests/test_integration.py::test_integration[minio_confpath-hrrr-kerchunk-concat-valid-time] XFAIL tests/test_integration.py::test_integration[minio_confpath-hrrr-kerchunk-concat-step] FAILED

raise NoCredentialsError()\nRuntimeError: botocore.exceptions.NoCredentialsError: Unable to locate credentials [while running \'Create|OpenWithKerchunk|WriteCombinedReference|Test dataset/WriteCombinedReference/WriteReference/Map(write_combined_reference)\']\n\n').returncode

hmm I'd guess something about remote_options/storage_options in the find reference file system not getting passed through to the result maybe?

This is actually great, insofar as it proves the value of the minio test! So much better to catch this here, rather than merge this and only find out in prod! 😃

I just rediscovered this bug since I didn't see the need to pass remote_options or target_options given we had dep injected target_root.

So the breaking change between 0.10.4 and what this PR merged into main is us removing this context manager that reused the auth credentials from dep injection

will file a bug later

…ltiZarrToZarr Transform

norlandrhagen · 2023-12-07T23:44:00Z

@sbquinlan @cisaacstern got the tests passing! If either one of you would mind taking one last look at the PR, it would be appreciated.

cisaacstern · 2023-12-08T00:05:48Z

.github/workflows/test-integration.yaml

-      - name: 🔁 Setup Python
-        id: setup-python
-        uses: actions/setup-python@v4
+
+      - name: Setup conda
+        uses: mamba-org/setup-micromamba@v1
        with:
-          python-version: ${{ matrix.python-version }}
-          cache: pip
-          cache-dependency-path: pyproject.toml
+          create-args: python==${{ matrix.python-version }}
+          environment-name: pangeo-forge
+
+      - name: Install pangeo-forge recipes and runner
+        shell: bash -l {0}
+        run: |
+          python -m pip install ${{ matrix.runner-version }}
+          python -m pip install -e ".[test,minio,grib]"


If we are using pip to install GRIB dependencies, do we need to be using micro mamba? Can't we just keep Setup Python the way it was?

Also, I'm somewhat surprised this works? The cf-grib/eccodes docs seems to suggest system-level dependencies need to be installed via conda?

Good call, just reverted that and the integration tests are passing.

cisaacstern

Another comment about how we're bringing in cfgrib for the test.

cisaacstern · 2023-12-08T00:26:22Z

.github/workflows/test-integration.yaml

-      - name: 🌈 Install pangeo-forge-recipes & pangeo-forge-runner
-        shell: bash -l {0}
-        run: |
-          python -m pip install ${{ matrix.runner-version }}
-          python -m pip install -e ".[test,minio]"


Sorry for the piecemeal review, @norlandrhagen.

Since we're not doing anything different re: pip, can we just preserve this block for a cleaner diff?

I would prefer to not support a user-facing grib optional dependency (at least as part of this PR), as I'm concerned that without putting that in a conda environment we can really ensure that will work reliably in a cross-platform way.

So I'd prefer just a separate workflow step here called "install grib deps" or similar, and not to have those deps as part of our pyproject.toml.

Seems like a good call!

If I understand.. I removed the grib deps from pyproject, reverted the install topython -m pip install -e ".[test,minio]" and added another step in the CI to install the grib deps via pip.

pyproject.toml

cisaacstern · 2023-12-08T00:31:32Z

Thanks for pushing this forward, @norlandrhagen, clearly we're very close.

My comments thus far are on packaging/GH workflows.

I haven't really digested the changes to the actual code. If @sbquinlan is able to take a look at that, great! If not, I can try to circle back to that section tomorrow.

…integration.yaml

norlandrhagen · 2023-12-08T00:34:49Z

Thanks for the CI feedback so far!

sbquinlan

Yeah I've looked over the code and tests. This all looks good to me. Nice work piping the storage and remote options all the way through.

norlandrhagen · 2023-12-08T02:06:21Z

Thanks for the review @sbquinlan !

cisaacstern · 2023-12-08T04:10:52Z

Awesome! If @sbquinlan is happy then I'm happy 😀 , great work @norlandrhagen and thanks for engaging so deeply on this, Sean.

norlandrhagen · 2023-12-08T04:41:46Z

Thank you both for the patience!
@sbquinlan hopefully adding the consolidate metadata transform a breeze!

norlandrhagen added 6 commits December 4, 2023 17:48

attempt 2 at PR

74e2e5d

wip

6db9817

tests passing

0e59878

updated test_end_to_end test_reference_grib to fix attr naming

2a4768f

fixed docstring

51e902d

removed pdb

8d02a29

norlandrhagen added input formats test-integration Apply this label to run integration tests on a PR. and removed input formats labels Dec 5, 2023

norlandrhagen added 11 commits December 5, 2023 14:52

apt eecodes

507f27a

big danger

56a7298

nit

f3d50d0

nit

cb734dd

micromamba install

5e33a7d

updated mm env steps

e8cd5f4

added env name

3da9b9f

typo

b8c2ffd

no deps

465de0a

typo nit

fa12b3f

remove no deps

a6268b2

sbquinlan reviewed Dec 5, 2023

View reviewed changes

norlandrhagen added 5 commits December 5, 2023 16:23

added WriteReference injection specs to injections.py

cdd15e8

bumped runner to two latest versions

9948fdb

added remote_options to mzz

345b978

explicit passing of target/remote options & remote protocol to any Mu…

0605699

…ltiZarrToZarr Transform

fixed integration test import + pdb removed

3e00fce

cisaacstern reviewed Dec 8, 2023

View reviewed changes

norlandrhagen added 2 commits December 7, 2023 17:17

remove conda test

ccb654b

typo

54f62b9

indent

e84142e

cisaacstern reviewed Dec 8, 2023

View reviewed changes

removed grib deps from pyproj and seperated out grib install in test-…

6b45c03

…integration.yaml

norlandrhagen mentioned this pull request Dec 8, 2023

WriteCombinedReference returns zarr.storage.FSStore #636

Closed

sbquinlan approved these changes Dec 8, 2023

View reviewed changes

cisaacstern merged commit 6e40279 into pangeo-forge:main Dec 8, 2023

ranchodeluxe mentioned this pull request Jan 10, 2024

Failing: TRMM NASA-IMPACT/veda-pforge-job-runner#16

Closed

4 tasks

abarciauskas-bgse mentioned this pull request Jan 12, 2024

Failing: MUR SST NASA-IMPACT/veda-pforge-job-runner#15

Closed

8 tasks

This was referenced Jan 15, 2024

Passing: GPM MERGIR NASA-IMPACT/veda-pforge-job-runner#17

Closed

write_combined_reference on main doesn't reuse dep injected FSSpecTarget credentials #666

Closed

Reuse FSSpecTarget Auth Credentials #668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kerchunk WriteCombinedReference return fsstore #660

Kerchunk WriteCombinedReference return fsstore #660

norlandrhagen commented Dec 5, 2023

sbquinlan Dec 5, 2023

cisaacstern Dec 5, 2023

sbquinlan Dec 5, 2023

norlandrhagen Dec 5, 2023

norlandrhagen Dec 7, 2023

sbquinlan Dec 5, 2023

sbquinlan Dec 5, 2023

norlandrhagen Dec 5, 2023

sbquinlan Dec 6, 2023

cisaacstern Dec 6, 2023

norlandrhagen Dec 6, 2023

sbquinlan Dec 6, 2023

cisaacstern Dec 6, 2023

norlandrhagen Dec 6, 2023

ranchodeluxe Jan 14, 2024 •

edited

Loading

norlandrhagen commented Dec 7, 2023

cisaacstern Dec 8, 2023

cisaacstern Dec 8, 2023

norlandrhagen Dec 8, 2023

cisaacstern left a comment

cisaacstern Dec 8, 2023

norlandrhagen Dec 8, 2023

norlandrhagen Dec 8, 2023

cisaacstern commented Dec 8, 2023

norlandrhagen commented Dec 8, 2023

sbquinlan left a comment

norlandrhagen commented Dec 8, 2023

cisaacstern commented Dec 8, 2023

norlandrhagen commented Dec 8, 2023

Kerchunk WriteCombinedReference return fsstore #660

Kerchunk WriteCombinedReference return fsstore #660

Conversation

norlandrhagen commented Dec 5, 2023

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

ranchodeluxe Jan 14, 2024 • edited Loading

Choose a reason for hiding this comment

norlandrhagen commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cisaacstern 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

cisaacstern commented Dec 8, 2023

norlandrhagen commented Dec 8, 2023

sbquinlan left a comment

Choose a reason for hiding this comment

norlandrhagen commented Dec 8, 2023

cisaacstern commented Dec 8, 2023

norlandrhagen commented Dec 8, 2023

ranchodeluxe Jan 14, 2024 •

edited

Loading