feat(datasets): Add NetCDFDataSet class

[riley-brady]

Description

There's a large geoscience/astrophysics and beyond community that leverage NetCDF for datasets that are stored with structured coordinates and metadata. A massive trove of the existing climate and weather data exists as NetCDF (.nc) files, for example.

See #165.

Development notes

I'd like to get this implemented with file syncing for load-from-remote, since it's most straight-forward. A future PR could work with kerchunk to allow direct loading from remote storage. This is a really nice toolkit, but requires management of a lot of JSON metadata files that are generated, and which sometimes can be quite slow to generate. It will take a little bit of tweaking to implement this nicely, since the first run would need to generate and cache/store all of the reference JSONs to make future loads much much faster.

Checklist

• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the relevant RELEASE.md file
• Added tests to cover my changes

[riley-brady]

Tested with AWS and GCS, but not sure if this is generalized enough. get_filepath_str consistently drops the prefix of URI's from object storage, which is problematic for the subsequent fs.get

[astrojuanlu]

Yeah filepath mangling in fsspec is tricky... we have a related open issue about this kedro-org/kedro#3196

[merelcht]

Thank you for opening this PR as contribution @riley-brady ! I had a quick look and left some minor comments. The main being that we've now renamed all our datasets to end in Dataset instead of DataSet, so that's something that needs to be changed here.

Would it be helpful if I post in our Slack channel to see if there's more interest from the community for this dataset?

[riley-brady]

Would it be helpful if I post in our Slack channel to see if there's more interest from the community for this dataset?

Thanks so much @merelcht! There's a lot of interest at #165 and I slacked with @astrojuanlu. It would be great if you posted on slack as well.

I have added your feedback and finished out and tested locally the PR. Going to add unit testing now and it should be ready for a thorough review. I'll update on that issue thread when the tests are added.

[riley-brady]

@merelcht the PR is fully implemented with testing and is ready for final review.

[astrojuanlu]

I spent a decent chunk of time today trying to understand what's breaking in the credentials test, and I still don't know what's going on. That test is literally copy-pasted from test_parquet_dataset.py, and yet it fails.

It's not clear at all what the test is doing though. The "Failed while loading data from data set NetCDFDataset" is hiding a quite puzzling error, that can be seen by enabling the logs:

$ pushd kedro-datasets && pytest -s --log-cli-level=DEBUG -vvv -k "pass_credentials and netcdf" --pdb; popd
...
DEBUG    s3fs:core.py:337 CALL: get_object - () - {'Bucket': 'test_bucket', 'Key': 'test.nc'}
                    DEBUG    Event before-parameter-build.s3.GetObject: calling handler <function sse_md5 at 0x136c36670>                                                                                                                                                                                    hooks.py:63
...
[02/15/24 14:42:42] DEBUG    Response headers: HTTPHeaderDict({'x-amz-request-id': 'C36M0ESFJX55TN4N', 'x-amz-id-2': 'b4c9yMZKWWSis5iwwE8suoOo3o5jO6LF/xxDnZqGFZwOinzNj+N+TIBHQQ3DaAc1Z6YkwPOo83I=', 'content-type': 'application/xml', 'transfer-encoding': 'chunked', 'date': 'Thu, 15 Feb 2024         parsers.py:239
                             13:42:42 GMT', 'server': 'AmazonS3'})                                                                                                                                                                                                                                                      
DEBUG    botocore.parsers:parsers.py:239 Response headers: HTTPHeaderDict({'x-amz-request-id': 'C36M0ESFJX55TN4N', 'x-amz-id-2': 'b4c9yMZKWWSis5iwwE8suoOo3o5jO6LF/xxDnZqGFZwOinzNj+N+TIBHQQ3DaAc1Z6YkwPOo83I=', 'content-type': 'application/xml', 'transfer-encoding': 'chunked', 'date': 'Thu, 15 Feb 2024 13:42:42 GMT', 'server': 'AmazonS3'})
                    DEBUG    Response body:                                                                                                                                                                                                                                                               parsers.py:240
                             <coroutine object AioAWSResponse._content_prop at 0x2bdbd1f40>                                                                                                                                                                                                                             
DEBUG    botocore.parsers:parsers.py:240 Response body:
<coroutine object AioAWSResponse._content_prop at 0x2bdbd1f40>
                    DEBUG    Nonretryable error: a bytes-like object is required, not 'coroutine'                                                                                                                                                                                                            core.py:125
DEBUG    s3fs:core.py:125 Nonretryable error: a bytes-like object is required, not 'coroutine'
FAILED

At what point in the stack is the create_client method supposed to be called, I have no idea.

Other datasets take a different approach and instead create a fake bucket, for example Spark and Video datasets.

@pytest.fixture
def mocked_s3_bucket():
    """Create a bucket for testing using moto."""
    with mock_s3():
        conn = boto3.client(
            "s3",
            region_name="us-east-1",
            aws_access_key_id=AWS_CREDENTIALS["key"],
            aws_secret_access_key=AWS_CREDENTIALS["secret"],
        )
        conn.create_bucket(Bucket=S3_BUCKET_NAME)
        yield conn

This PR has been around for 4 months and is almost there. Is there any reasonable way we can take over from @riley-brady and get it merged?

[astrojuanlu]

That test is literally copy-pasted from test_parquet_dataset.py, and yet it fails.

Well, and now I realized that it was marked as xfail in #463 and #459 after @merelcht spent one week trying to understand what to do with it. We just fell in the same trap again...

@ankatiyar has done a fair share of debugging as well last week.

I'm marking the one on NetCDF as xfail too.

[astrojuanlu]

yeah the tests are broken. I'm taking this to another branch to do some debugging.

[riley-brady]

Thanks for all the work on this, everyone.

[galenseilis]

Thanks for all the work on this, everyone.

I'd like to echo that sentiment. As I PyMC user I often produce NetCDF files from xarrays. I really appreciate the efforts that have gone into this so far, and I am looking forward to PyMC integrating better with Kedro. :)

[noklam]

@galenseilis what would you like to see integrated better with kedro particularly?

[galenseilis]

@galenseilis what would you like to see integrated better with kedro particularly?

I just meant the existence of NetCDFDataSet. NetCDF is the conventional choice for saving PyMC modelling results. I'm excited about that! 🤩

[merelcht]

For transparency on why this PR isn't merged yet: there's some issues with the tests, they seem to just hang and it's not clear why. @ankatiyar is currently investigating the issue and finding a way to maybe re-write the tests. Thank you so much for your work and patience @riley-brady, the Kedro team will make sure this gets merged in asap.

[ankatiyar]

I've made some changes and skipped the tests that load the dataset from s3 because those are getting stuck.

[astrojuanlu]

I love that this logic is simpler now 💯

[astrojuanlu]

Left only 1 question @ankatiyar , if it's okay proceed and merge

[riley-brady]

Thanks everyone!!