TRT-635: Migrate GDAL commands to Python bindings. by owenlittlejohns · Pull Request #42 · nasa/harmony-gdal-adapter

owenlittlejohns · 2025-10-02T02:33:26Z

Description

Alright then. This is the big one! (Or at least one of the things we've been talking about for the longest)

This PR converts requests to GDAL over to using the available Python bindings, rather than executing shell commands directly. I'll leave some comments on the PR, but I think a couple of really helpful links:

Documentation on the Python bindings (opening at Translate).
Documentation on the raw GDAL commands (opening at gdal_translate).

Also, I must admit, I initially leant on the Google AI generated answer for how to convert ogr2ogr to Python, but the options in VectorTranslateOptions gave me a bit of confidence.

I think we're also possibly missing some regression tests - looks like we don't really test reprojection or shape file workflows.

Jira Issue ID

TRT-635 - the neverending story

Local Test Steps

Pull this branch. Execute the unit tests: ./bin/build-image && ./bin/build-test && ./bin/run-test. All tests should pass.
Fire up an instance of Harmony in a Box, with LOCALLY_DEPLOYED_SERVICES=harmony-gdal-adapter
Run through the HGA Regression tests, by:
- Creating the papermill-hga conda environment: conda env create -f hga/environment.yml.
- Activating that environment: conda activate papermill-hga.
- Starting a Jupyter notebook server: jupyter notebook.
- Navigating in your browser to the HGA regression test notebook.
- Setting harmony_host_url='http://localhost:3000'.
- Running all tests. They should pass.
Confirm the service version has been added to the output file:
- Go to http://localhost:3000/jobs and pick a job with a GeoTIFF output.
- Download the output file.
- Run gdalinfo on that output.
- Look at the gdalinfo output in your terminal to find a line:

gdal_subsetter_version=3.0.5

(This will be in the Metadata section, a little bit above the corner points.)

PR Acceptance Checklist

~~Jira ticket acceptance criteria met.~~
version.txt and CHANGELOG.md updated if any service code is changed.
Tests added/updated and passing.
~~Documentation updated (if needed).~~

flamingbear

This is great, so much nicer. But I see at least one typo and had a question about using process from the Format.

flamingbear · 2025-10-06T16:46:57Z

-    def execute_gdal_command(self, *args) -> list[str]:
-        """This is used to execute gdal* commands."""
-        self.logger.info(
-            args[0] + " " + " ".join(["'{}'".format(arg) for arg in args[1:]])
-        )
-        result_str = check_output(args).decode("utf-8")
-        return result_str.split("\n")


flamingbear · 2025-10-06T16:56:47Z

+    def resize(self, layerid: str, srcfile: str, dstdir: str) -> str:
+        """Resizes the input layer and returns the name of the output file.

        Uses a call to gdal_translate using information in message.format.


It doesn't do this anymore.

Well, it kind of does - osgeo.gdal.Translate is just a binding around gdal_translate - but I can improve the documentation string.

Updated in 62f49ef.

I definitely thought gdal_translate was the executable. I didn't realize osgeo.gdal.Translate actually called it.

flamingbear · 2025-10-06T17:00:12Z

        Uses a call to gdal_translate using information in message.format.

        """
-        interpolation = self.message.format.process("interpolation")


Doesn't process mark this as being "used" or something? I thought process was a way to remove things from a message before downstream services use it. I only have a vague idea about this and I'm sure you told me. But just looking at this change and want to make sure we're not breaking something.

You're right - process is meant to do that. From what I can tell:

Using Message.format.process will drop that property from the Message.output_data dict.

The output message from the service is then derived from the Message.output_data dict after processing

So:

My first concern was that the message wouldn't be able to reuse the same property within the service, and some of the message properties are used multiple times (CRS, for example). This is irrelevant. Message.output_data won't preclude parameter reuse in the same service.

Do we want to strip things out of the message? I'm honestly not sure. I could see times when it's good (preventing repeat processing) and times when it's bad (the same parameter being needed in multiple services).

I guess I don't have an answer for what we should do, but definitely acknowledge this is now doing something different. What are your thoughts?

I don't have strong opinions. This used to remove an "interpolation" value so it wouldn't be available to downstream services, but this is the service that should do interpolation probably. I don't think a lot of services are using the process mechanism. But I also don't know if they should be.

How is that for a non-answer?

😆

I added a disclaimer in the method and function documentation strings that makes it clear the parameters aren't being removed in b37c762.

flamingbear · 2025-10-06T17:06:09Z

+                resampleAlg=resample_algorithm,
+                height=output_height,
+                width=output_width,
+            )


~~I think you're missing the unscale option from L476?~~ (nevermind, we stopped supporting PNGs)

flamingbear · 2025-10-06T17:17:58Z

+
+    interpolation = rgetattr(harmony_message, "format.interpolation", None)
+
+    if interpolation in get_args(ResampleAlgorithms):


I definitely had to google get_args

Yeah - I think we might have used it in xarray when we were doing the DataTree migration, but it's not my most familiar go-to.

owenlittlejohns · 2025-10-06T22:00:36Z

The unit tests failing are... interesting. The failures are imports of _gdal from inside the osgeo dependency. I'd not changed that, and I can't see a failure in the installation of GDAL in the CI/CD logs. Given that I hadn't changed anything in the environment between the last time the tests passed and this commit, I'm a bit surprised, if I'm honest.

Update: The conda install command in service.Dockerfile was still trying to get gdal from the defaults channel. It now retrieves it from conda-forge and that seems to have made things work again. I'll not pretend to understand the difference between the two versions, though.

flamingbear

I don't have opinions on the change to the message format behavior. But if you leave it maybe note it as a difference in behavior? Thanks for fixing the other things. I do think keeping this smaller and adding tests and refactoring should be done in another PR.

flamingbear · 2025-10-07T17:46:51Z


 # Install GDAL
-RUN conda run --name hga conda install gdal=3.6.2
+RUN conda run --name hga conda install gdal=3.6.2 --channel conda-forge --override-channels


That's a head scratcher that I won't get pulled into. But good catch.

owenlittlejohns · 2025-10-07T19:24:44Z

I do think keeping this smaller and adding tests and refactoring should be done in another PR.

Yup. I think that would be the best target for the next PR. (Not sure whether that comes before or after adding regression tests for projection. But these feel like the two next things to do)

TRT-635: Migrate GDAL commands to Python bindings.

9308f57

owenlittlejohns requested a review from flamingbear as a code owner October 2, 2025 02:33

owenlittlejohns commented Oct 2, 2025

View reviewed changes

Comment thread gdal_subsetter/transform.py Outdated

Comment thread gdal_subsetter/transform.py Outdated

owenlittlejohns commented Oct 2, 2025

View reviewed changes

Comment thread gdal_subsetter/transform.py

TRT-635: Fix bilinear typo.

a1dcef9

owenlittlejohns added the hacktoberfest-accepted label Oct 3, 2025

flamingbear requested changes Oct 6, 2025

View reviewed changes

owenlittlejohns added 2 commits October 6, 2025 17:28

TRT-635: Fix typo, add type hint and improve documentation strings.

62f49ef

TRT-635: Explicitly set conda nodefaults.

56bef34

TRT-635: Make sure conda install doesn't use defaults channel.

3d11c28

owenlittlejohns requested a review from flamingbear October 6, 2025 22:15

flamingbear approved these changes Oct 7, 2025

View reviewed changes

TRT-635: Add note about Harmony message parameter retrieval.

b37c762

owenlittlejohns merged commit 2d097ea into main Oct 7, 2025
4 checks passed

owenlittlejohns deleted the TRT-635-remove-shell-command-execution branch October 7, 2025 19:25


		interpolation = rgetattr(harmony_message, "format.interpolation", None)

		if interpolation in get_args(ResampleAlgorithms):

Conversation

owenlittlejohns commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Jira Issue ID

Local Test Steps

PR Acceptance Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flamingbear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owenlittlejohns commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flamingbear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owenlittlejohns commented Oct 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

owenlittlejohns commented Oct 2, 2025 •

edited

Loading

owenlittlejohns commented Oct 6, 2025 •

edited

Loading