DAS-1376 - Update HarmonyAdapter to perform many-to-one operations. #18

owenlittlejohns · 2022-02-19T01:08:30Z

This PR updates the main NetCDFToZarrAdapter class with a view to many-to-one operations. This includes adding utilities for downloading multiple granules in parallel threads, and for combining the input STAC items into a single output stack item that describes the aggregated output.

A lot of this work is heavily inspired by some great, similar functionality in the PO.DAAC Concise service.

For now, I've tried to ensure a single granule request could still operate, but a multiple granule request will raise an exception, because we've not yet implemented that functionality.

owenlittlejohns · 2022-02-19T01:10:29Z

requirements/dev.txt

@@ -1,5 +1,5 @@
 autopep8 ~= 1.5
-coverage ~= 4.5
+coverage ~= 6.3.1


Interesting thing here - the older version of coverage was causing test failures for the multiprocessing module. I think this is the related GitHub issue.

owenlittlejohns · 2022-02-19T01:13:14Z

harmony_netcdf_to_zarr/stac_utilities.py

@@ -0,0 +1,126 @@
+""" A module containing utility functions to extract information from
+    `pystac.Item` instances for Harmony, or to produce an output
+    `pystac.Catalog`. These functions adapt work from the PO.DAAC concise


So these aren't identical - but they really do lean heavily on the great work done by @frankinspace et al. (Same for the download_utilities.py module. There might be some stuff that could be migrated into harmony-service-lib-py (like multi-threaded downloads for many-to-one services). But (as David Auty likes to say) those are "future distractions".

100% on multi-threaded downloads. That absolutely should be a shared library - eventually. Or perhaps even better - moved into the HarmonyPy package?

owenlittlejohns · 2022-02-19T01:14:20Z

harmony_netcdf_to_zarr/stac_utilities.py

+                None)
+
+
+def get_output_catalog(input_catalog: Catalog, zarr_root: str) -> Catalog:


I'm also now wondering if we need to have similar code in HOSS - particularly for requests that specify a bounding box.

As in get_output_bounding_box (below) - So much common code, Harmony Adapter, or HarmonyPy ? Agree we may need to consider a HOSS ticket.

I think the harmony-service-lib-py package is probably the place to pop some of this stuff. That's the core Python package that offers common functionality to the backend services, including the HarmonyAdapter. It's very tempting to just suggest copying this module (almost line-for-line) into that package.

owenlittlejohns · 2022-02-19T01:16:09Z

tests/unit/test_download_utilities.py

+            be caused by requesting a file with an unknown protocol.
+
+        """
+        with self.assertRaises(RuntimeError):


This test has been a little flaky on my local machine (the harmony.util.download function is successful for a URL that isn't an obvious local file path or URL). I'm not sure I can see why. I'm hoping it behaves on GitHub.

The problem was that, in the first failure, the local file /tmp/<uuid>.nc4 was being created, even in failure. The harmony.util.download functions sees this file in subsequent test runs.

I've updated this test to put test artefacts in a temporary directory, and then remove that directory in the TestCase.tearDown method.

owenlittlejohns · 2022-02-19T01:19:42Z

I looked at the test failures. It looks like the end-to-end tests. Probably due to swapping out the process_item for process_items_many_to_one. I'll work on fixing that up.

Update: I also added validation to ensure a STAC catalog was included in the input. The end-to-end tests didn't have one in the arguments. I've tried updating the end-to-end tests with a --harmony-sources argument... we'll see how those go.

owenlittlejohns · 2022-02-22T18:55:15Z

tests/test_adapter.py


+    @patch('harmony_netcdf_to_zarr.adapter.download_granules')


I'm a little bit disappointed to have to mock the download of the input granule as that makes this (and the large file conversion) less of an end-to-end test. However, the mock used to make the multiprocessing module work with moto was then causing issues with the multiprocessing.Queue used to download the input granules.

I spent quite a while looking into this - there was some odd behaviour, indeed! Mocking the download in these two tests was the simplest way I could find to make the tests work in the short term.

kcockeri · 2022-02-24T18:50:32Z

harmony_netcdf_to_zarr/adapter.py

+                # Mimicking PO.DAAC Concise: for many-to-one the file name is
+                # "<collection>_merged.zarr".
+                collection = self._get_item_source(items[0]).collection
+                output_name = f'{collection}_merged.zarr'


I assume {collection} is concept_id, not just short_name.

I had to dig around for this in the unit tests for harmony-service-lib-py to confirm. Looking at the unit tests there, it looks like you are right; the URL will likely contain the collection concept ID, not the short name. (Here's the class method, where it's essentially looking for the parent item in the STAC catalogue).

Short-Name is preferable for end-user. Is this possible?

I definitely agree that a short name is more end-user friendly! I'm not sure that the short name is available to us - this is making use of a method on the BaseHarmonyAdapter class, so outside of the scope of the service itself.

I guess the other thing worth noting is that this is what the PO.DAAC Concise service is doing, so we're being consistent with existing services. I definitely think this is a good question, and think the short name would be preferable if available. This might be a conversation to float up to a higher level, to see if the collection short name can be made available to the services somehow.

Addendum (sorry, still mulling this over): I think, mainly, this all stems from what Harmony chooses to put in the input STAC catalogue sent to the backend service.

(An alternative might be to delve into the metadata of one of the granules to find a short name in the global attributes, but then we'd be inconsistent with Concise)

kcockeri · 2022-02-24T18:52:14Z

harmony_netcdf_to_zarr/download_utilities.py

+                      access_token: str, harmony_config: Config,
+                      logger: Logger, process_count: int = None) -> List[str]:
+    """ A method which scales concurrent downloads to the number of available
+        CPU cores. For further explaination, see documentation on "multi-track


explanation

Gah. That typo is just embarrassing. I'll fix that up! Thanks, Ken.

Update: I've fixed this typo.

D-Auty · 2022-02-25T16:10:42Z

harmony_netcdf_to_zarr/adapter.py

+                # Temporarily retain old version of NetCDF-to-Zarr service
+                netcdf_to_zarr(local_file_paths[0], zarr_store)
+            else:
+                raise NotImplementedError('Concatenation is not yet supported.')


I'm confused - isn't this where we initiate our aggregation processing? Above code addresses many-to-one options, why raise an exception here? Oh - I got it, from some conversation - this is still a mergeable solution but not the final solution (yet to come). Classic case for a ToDo comment, but not strictly necessary.

Yeah exactly. My aim here was to ensure that we could deploy this and continue service for a single granule input, but we've not yet actually done the bit to write many-to-one input (that's DAS-1379).

I've added a short-lived TODO comment referring to DAS-1379.

D-Auty · 2022-02-25T16:28:24Z

harmony_netcdf_to_zarr/stac_utilities.py

@@ -0,0 +1,126 @@
+""" A module containing utility functions to extract information from
+    `pystac.Item` instances for Harmony, or to produce an output
+    `pystac.Catalog`. These functions adapt work from the PO.DAAC concise


100% on multi-threaded downloads. That absolutely should be a shared library - eventually. Or perhaps even better - moved into the HarmonyPy package?

D-Auty · 2022-02-25T16:34:30Z

harmony_netcdf_to_zarr/stac_utilities.py

+                None)
+
+
+def get_output_catalog(input_catalog: Catalog, zarr_root: str) -> Catalog:


As in get_output_bounding_box (below) - So much common code, Harmony Adapter, or HarmonyPy ? Agree we may need to consider a HOSS ticket.

D-Auty · 2022-02-25T16:37:03Z

tests/test_adapter.py

+        objects are gone when the child processes quit. This method mocks
+        `multiprocessing.popen_fork.Popen._launch` to do the work in the
+        parent process.
+


D-Auty · 2022-02-25T16:38:14Z

tests/test_adapter.py


+    @patch('harmony_netcdf_to_zarr.adapter.download_granules')


owenlittlejohns commented Feb 19, 2022

View reviewed changes

owenlittlejohns force-pushed the DAS-1376-multiple-input-granules branch 3 times, most recently from 4898523 to a3385cf Compare February 19, 2022 17:24

goktugerol-dev approved these changes Feb 20, 2022

View reviewed changes

owenlittlejohns force-pushed the DAS-1376-multiple-input-granules branch 2 times, most recently from 7950ce9 to 44d62d5 Compare February 22, 2022 18:51

owenlittlejohns commented Feb 22, 2022

View reviewed changes

kcockeri reviewed Feb 24, 2022

View reviewed changes

kcockeri approved these changes Feb 24, 2022

View reviewed changes

D-Auty reviewed Feb 25, 2022

View reviewed changes

DAS-1376 - Update HarmonyAdapter to perform many-to-one operations.

1e62fa1

owenlittlejohns force-pushed the DAS-1376-multiple-input-granules branch from 44d62d5 to 1e62fa1 Compare February 25, 2022 16:35

D-Auty reviewed Feb 25, 2022

View reviewed changes

owenlittlejohns merged commit 9e1b8b0 into main Feb 25, 2022

owenlittlejohns deleted the DAS-1376-multiple-input-granules branch February 25, 2022 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAS-1376 - Update HarmonyAdapter to perform many-to-one operations. #18

DAS-1376 - Update HarmonyAdapter to perform many-to-one operations. #18

owenlittlejohns commented Feb 19, 2022

owenlittlejohns Feb 19, 2022

owenlittlejohns Feb 19, 2022

D-Auty Feb 25, 2022

owenlittlejohns Feb 19, 2022

D-Auty Feb 25, 2022

owenlittlejohns Feb 25, 2022

owenlittlejohns Feb 19, 2022

owenlittlejohns Feb 19, 2022

owenlittlejohns commented Feb 19, 2022 •

edited

owenlittlejohns Feb 22, 2022

D-Auty Feb 25, 2022

kcockeri Feb 24, 2022

owenlittlejohns Feb 25, 2022 •

edited

D-Auty Feb 25, 2022

owenlittlejohns Feb 25, 2022

owenlittlejohns Feb 25, 2022

kcockeri Feb 24, 2022

owenlittlejohns Feb 25, 2022 •

edited

D-Auty Feb 25, 2022 •

edited

owenlittlejohns Feb 25, 2022

owenlittlejohns Feb 25, 2022

D-Auty Feb 25, 2022

D-Auty Feb 25, 2022

D-Auty Feb 25, 2022

D-Auty Feb 25, 2022

		None)


		def get_output_catalog(input_catalog: Catalog, zarr_root: str) -> Catalog:

DAS-1376 - Update HarmonyAdapter to perform many-to-one operations. #18

DAS-1376 - Update HarmonyAdapter to perform many-to-one operations. #18

Conversation

owenlittlejohns commented Feb 19, 2022

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 commented Feb 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenlittlejohns Feb 25, 2022 • edited

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 25, 2022 • edited

Choose a reason for hiding this comment

D-Auty Feb 25, 2022 • edited

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 commented Feb 19, 2022 •

edited

owenlittlejohns Feb 25, 2022 •

edited

owenlittlejohns Feb 25, 2022 •

edited

D-Auty Feb 25, 2022 •

edited