TRT-571 - Update net2cog to process multiple variables. #38

owenlittlejohns · 2025-02-03T18:38:15Z

Github Issue: #32 (also one of the items mentioned in #35).

Description

This PR adds support for requests asking for multiple variables, either explicitly, or when a Harmony request specifies "all".

Overview of work done

There's a fair bit of shuffling around, but essentially the code changes are:

If there is an empty list specified for variables, this is an "all" request, and so do all data variables.
The _write_cogtiff function has been rescoped to just write a single COG. The iteration now takes place in netcdf_converter.
I implemented the generate_output_filename from the harmony-service-lib-py module, which now has a reformatted suffix (note, this is a minor difference from converted). I removed a bunch of custom code that was geared around generating the output filename, as that is no longer needed.
I consolidated test fixtures into a conftest.py.

Overview of verification done

Updated unit tests (including consolidating test fixtures into tests/conftest.py). There are now tests that run to confirm multiple variables can be processed, either via explicitly requesting them or by using "all" in a Harmony request. These tests exist at the overall service level, and the netcdf_converter function level.
I also have updated the net2cog regression tests (see: TRT-571 - Update regression tests for net2cog. nasa/harmony-regression-tests#131)

Overview of integration done

Tested via the examples in the regression tests (see: TRT-571 - Update regression tests for net2cog. nasa/harmony-regression-tests#131).

PR checklist:

Linted
Updated unit tests
Updated changelog
Integration testing

See Pull Request Review Checklist for pointers on reviewing this pull request

net2cog/netcdf_convert.py

owenlittlejohns · 2025-02-03T18:46:23Z

net2cog/netcdf_convert.py

+            raise Net2CogError(error) from error
+        except LookupError as err:
+            logger.info("Variable %s cannot be converted to tif: %s", variable_name, err)
+            return None


I'm not sure that return None is the best thing for each of these cases, but it was more of an equivalent to the previous continue when there was a loop in this function. I'd probably prefer to raise an exception instead in a few of these return None instances. What do you think @frankinspace?

Yes I think it makes sense to raise an exception here. Perhaps create a custom FailedConversion exception or similar so that we could in the future add custom error handling in netcdf_convert_harmony

279641

Currently the Net2CogError was only being raised within this function, so I opted to use that. Happy to make a child of that exception type, though, if that feels better.

net2cog/netcdf_convert.py

owenlittlejohns · 2025-02-03T18:50:20Z

net2cog/netcdf_convert.py

+            if not var_list:
+                # Empty list means "all" variables, so get all variables in
+                # the `xarray.Dataset`.
+                var_list = list(xds.data_vars.keys())


This is something I wanted to call out.

I think using xarray.Dataset.data_vars will essentially prevent an "all" request from trying to convert coordinate variables, so it feels like the right thing to choose, instead of all varibles.

That said, if a user specifies the coordinate variables by name, we'll still hit the code that checks against EXCLUDED_VARIABLES. Maybe it would be more scaleable to check the xarray.Dataset.coordinates instead?

This seems like a reasonable step forward. The EXCLUDED_VARIABLES was a pretty simplistic way of trying to deal with not converting coordinate variables so this does improve that IMO

Just to confirm, what we have in this PR is good for now, and perhaps swapping EXCLUDED_VARIABLES for xarray.Dataset.coordinates is something for future work? Happy either way, just making sure I'm not missing some work to do here.

net2cog/netcdf_convert_harmony.py

owenlittlejohns · 2025-02-03T18:54:40Z

net2cog/netcdf_convert_harmony.py

+            # Rename the downloaded output from a SHA256 hash to the basename
+            # of the source data file
+            source_asset_basename = os.path.basename(asset.href)
+            os.rename(input_filename, os.path.join(output_dir, source_asset_basename))
+            input_filename = os.path.join(output_dir, source_asset_basename)


I was a bit on the fence with this change. It feels a bit clunky.

When Harmony downloads a file, it assigns a SHA256 hash as the filename, based on the URL it got the file from. But, we want to be passing around the base name of the original granule, so the output file names can be generated for every variable. I was considering two options:

This: rename the downloaded file to have a base name that matches the source file, instead of the SHA 256 hash.

Pass around the source granule base name an additional variable.

I opted for a bit more complexity here, to avoid having another string floating around and being confusing.

May want to reconsider this approach given my previous comment about removing harmony dependency from netcdf_convert

Okay - if we aren't using the generate_output_filename in netcdf_convert.py, then I think we can avoid having to rename things. So this can be cleaned up to just use the SHA256-derived name.

I think the other part of it, though, is making sure that there is a mapping in the output from variable name to file path, so that we can generate the correct file name in netcdf_convert_harmony.py. If you are fine with tweaking the output of netcdf_convert.py::netcdf_converter to something like a dictionary, then I can work up something like that and move the renaming out to netcdf_convert_harmony.py.

Update: A simpler alternative - the default (non-Harmony) file name could just be <variable name>.tif.

Yeah I like <variable name>.tif. Do we need to worry about special characters there?

Do we need to worry about special characters there?

Great catch!

I had just run with using <variable_name>.tif in 279641, but hadn't accounted for the possibility of a slash (which will come up with hierarchical collections). I will at least do something to swap out slash characters for underscores and push that up. Should be a quick tweak, and I think that's compatible with what harmony-service-lib does to those characters as well.

There we go: 656f49

I just do a simple string replace for slashes.

net2cog/netcdf_convert_harmony.py

tests/conftest.py

frankinspace

Nice work, thank you for the contribution! Just a few comments to consider

frankinspace · 2025-02-03T20:22:17Z

net2cog/netcdf_convert.py

        xarray dataset loaded from NetCDF file
+    variable_name: str
+        Name of the variable as passed in to Harmony.


nit picky but shouldn't mention Harmony in the non-harmony module. Intent is that this netcdf_convert module could be used independently of Harmony.

Updated in 279641

net2cog/netcdf_convert.py

frankinspace · 2025-02-03T20:31:23Z

net2cog/netcdf_convert.py

+            raise Net2CogError(error) from error
+        except LookupError as err:
+            logger.info("Variable %s cannot be converted to tif: %s", variable_name, err)
+            return None


Yes I think it makes sense to raise an exception here. Perhaps create a custom FailedConversion exception or similar so that we could in the future add custom error handling in netcdf_convert_harmony

frankinspace · 2025-02-03T20:35:41Z

net2cog/netcdf_convert.py

+            if not var_list:
+                # Empty list means "all" variables, so get all variables in
+                # the `xarray.Dataset`.
+                var_list = list(xds.data_vars.keys())


This seems like a reasonable step forward. The EXCLUDED_VARIABLES was a pretty simplistic way of trying to deal with not converting coordinate variables so this does improve that IMO

frankinspace · 2025-02-03T20:46:48Z

net2cog/netcdf_convert_harmony.py

+            # Rename the downloaded output from a SHA256 hash to the basename
+            # of the source data file
+            source_asset_basename = os.path.basename(asset.href)
+            os.rename(input_filename, os.path.join(output_dir, source_asset_basename))
+            input_filename = os.path.join(output_dir, source_asset_basename)


May want to reconsider this approach given my previous comment about removing harmony dependency from netcdf_convert

…ony.py.

TRT-571 - Update net2cog to process multiple variables.

c6642f4

owenlittlejohns requested a review from frankinspace February 3, 2025 18:38

owenlittlejohns mentioned this pull request Feb 3, 2025

TRT-571 - Update regression tests for net2cog. nasa/harmony-regression-tests#131

Open

4 tasks

owenlittlejohns commented Feb 3, 2025

View reviewed changes

frankinspace reviewed Feb 3, 2025

View reviewed changes

owenlittlejohns added 4 commits February 3, 2025 17:06

TRT-571 - Migrate harmony naming functionality to netcdf_convert_harm…

2796419

…ony.py.

TRT-571 - Replace slashes in output file name with underscores.

656f49d

TRT-571 - Fix minor typo in CHANGELOG.md.

bae66fe

TRT-571 - Ensure variable name is from basename of variable file.

d27eabc

owenlittlejohns requested a review from frankinspace February 10, 2025 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRT-571 - Update net2cog to process multiple variables. #38

TRT-571 - Update net2cog to process multiple variables. #38

owenlittlejohns commented Feb 3, 2025 •

edited

Loading

owenlittlejohns Feb 3, 2025 •

edited

Loading

frankinspace Feb 3, 2025

owenlittlejohns Feb 3, 2025

owenlittlejohns Feb 3, 2025

frankinspace Feb 3, 2025

owenlittlejohns Feb 3, 2025

owenlittlejohns Feb 3, 2025

frankinspace Feb 3, 2025

owenlittlejohns Feb 3, 2025 •

edited

Loading

frankinspace Feb 3, 2025

owenlittlejohns Feb 3, 2025

owenlittlejohns Feb 3, 2025

frankinspace left a comment

frankinspace Feb 3, 2025

owenlittlejohns Feb 3, 2025

frankinspace Feb 3, 2025

frankinspace Feb 3, 2025

frankinspace Feb 3, 2025

TRT-571 - Update net2cog to process multiple variables. #38

Are you sure you want to change the base?

TRT-571 - Update net2cog to process multiple variables. #38

Conversation

owenlittlejohns commented Feb 3, 2025 • edited Loading

Description

Overview of work done

Overview of verification done

Overview of integration done

PR checklist:

owenlittlejohns Feb 3, 2025 • edited Loading

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

owenlittlejohns Feb 3, 2025 • edited Loading

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

frankinspace 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

owenlittlejohns commented Feb 3, 2025 •

edited

Loading

owenlittlejohns Feb 3, 2025 •

edited

Loading

owenlittlejohns Feb 3, 2025 •

edited

Loading