DAS-NONE: Random small fixes. #38

flamingbear · 2024-12-10T18:52:39Z

Description

No functional differences.

Service python version updated to 3.12.
Updates the test matrix to include python 3.12.
Replaces OS with Path in adapter.py.
Updates the pre-commit-config to remove dependency on black-jupyter.
Runs pre-commit on all files and commits those changes.
Simplifies the test scripts with pytest and adds a report directory for artifacts.

Jira Issue ID

DAS-None

Local Test Steps

Pull this branch build and run tests.

❯ ./bin/build-image && ./bin/build-test &&  ./bin/run-test

Verify the code still runs the current regressions by deploying the new hybig image to your local Harmony-In-A-Box and run the hybig regression tests.

You can additionally verify that the changes to the github actions creates a correct artifact containing the coverage and test report. https://github.com/nasa/harmony-browse-image-generator/actions/runs/12266465971?pr=38

PR Acceptance Checklist

[NA] Jira ticket acceptance criteria met.
CHANGELOG.md updated to include high level summary of PR changes.
[NO] docker/service_version.txt updated if publishing a release.
Tests added/updated and passing.
[NA] Documentation updated (if needed).

No functional differences.

Removes jupyter-black dependency.

I didn't know, but the message from the caught exception is not added directly to the raised exception.

This tweak is going wild.

flamingbear · 2024-12-10T23:29:10Z

I verified everything and I'm about to work from here to fix the rgb[a] images from DAS-2276 and I will release the changes together.

flamingbear · 2024-12-10T23:32:52Z

docs/HyBIG-Example-Usage.ipynb

Just changed 2 comments for line length.

flamingbear · 2024-12-10T23:33:39Z

harmony_service/adapter.py

@@ -145,7 +145,7 @@ def process_item(self, item: Item, source: HarmonySource) -> Item:

        except Exception as exception:
            self.logger.exception(exception)
-            raise HyBIGServiceError from exception
+            raise HyBIGServiceError(str(exception)) from exception


This actually is a change before we logged the correct exception and raised a new one from the old, but the workflow-ui didn't show any information about it.

flamingbear · 2024-12-10T23:44:14Z

tests/test_service/test_adapter.py

+            HyBIGServiceError,
+            ('You are forbidden to download.'),
+        ):
+            hybig.invoke()


This tests that an error raised in the harmony-service-lib is raised as a HyBIGServiceError and has the correct information attached. This is displayed in the workflow-ui

flamingbear · 2024-12-10T23:44:52Z

tests/unit/test_crs.py

@@ -54,7 +54,7 @@
 )

 WKT_EPSG_6050 = dedent(
-    '''
+    """


what animal used three single quotes? 🪞

flamingbear · 2024-12-10T23:47:55Z

tests/unit/test_sizes.py

-            self.assertEqual(actual_scale_extent, expected_scale_extent)
+            assert expected_scale_extent == pytest.approx(
+                actual_scale_extent, rel=1e-12
+            )


This showed up with a 14th decimal place difference between python versions.

flamingbear · 2024-12-10T23:58:07Z

tests/run_tests.sh

@@ -20,28 +20,19 @@ STATUS=0

 export HDF5_DISABLE_VERSION_CHECK=1


This line doesn't seem necessary. I wonder if it's here from another project...

Yeah, this is some straight-up cargo cult stuff from some older version of this script which has been copied and pasted from project to project. Agree with cutting it. (One day this file will be a glorious one liner that just becomes part of the CI/CD directly)

vutrannasa · 2024-12-11T14:25:30Z

harmony_service/utilities.py

@@ -21,7 +21,7 @@ def get_tiled_file_extension(file_name: Path) -> str:
    generate the correct one to pass in.

    """
-    ext_pattern = r"(\.r\d+c\d+)?\.(png|jpg|pgw|jgw|txt)(.aux.xml)?"
+    ext_pattern = r'(\.r\d+c\d+)?\.(png|jpg|pgw|jgw|txt)(.aux.xml)?'


Why change double quote to single quote?
Consistently?
Coding standards?

Both of those things. I don't have a preference, but I couldn't convince Owen to go to Black style formatting with the double quotes. I'm not actually sure how all of these became double quotes in the past because we did have some style checks that I thought would have caught them. The new ruff-formatting added and used by pre-commit.ci, runs on all files, and found all of the places where I just accidentally left double quotes in.

So tl;dr Yes, consistency and coding standards.

ok, I looked, before we used black-jupyter with "skip-string-normalization" which just leaves strings however you put them in, and doesn't change them. the pyproject.toml specifies "single" for quote style to force you to choose one. (consistency)

I couldn't convince Owen to go to Black style formatting with the double quotes.

😅

before we used black-jupyter with "skip-string-normalization" which just leaves strings however you put them in

Yup, this.

Why change double quote to single quote?
Consistently?
Coding standards?

I think somewhere in PEP008 there's a sentence about whether to use single or double quotes. The sentiment is basically: it doesn't really matter which, but pick one and use it consistently.

Just simplify.

vutrannasa · 2024-12-11T15:46:08Z

tests/run_tests.sh

-    echo "ERROR: unittest generated errors"
-fi
+# Exit status used to report back to caller
+STATUS=0


Looking at the raw file
https://github.com/nasa/harmony-browse-image-generator/blob/8bf518b4a1af610f408b13fcb7d3e9356e44a01b/tests/run_tests.sh

Double declaration of STATUS=0 on Line 19 Line 25

vutrannasa · 2024-12-11T15:47:35Z

tests/run_tests.sh

+pytest --cov=hybig --cov=harmony_service \
+       --cov-report=html:reports/coverage \
+       --cov-report term \
+       --junitxml=reports/test-reports/test-results-"$(date +'%Y%m%d%H%M%S')".xml || STATUS=1


Not sure if STATUS=1 is set from code above.

This is where we set it when the test fails so that the exit code is non-zero on failures.

owenlittlejohns

Looks good to me.

I mentioned in a comment lower down, but I think bumping the Python version of the service is possibly worth a release all of it's own. I know that increases overhead a little bit (with testing, etc), so I'll let you make that call.

owenlittlejohns · 2024-12-11T23:40:44Z

harmony_service/utilities.py

@@ -21,7 +21,7 @@ def get_tiled_file_extension(file_name: Path) -> str:
    generate the correct one to pass in.

    """
-    ext_pattern = r"(\.r\d+c\d+)?\.(png|jpg|pgw|jgw|txt)(.aux.xml)?"
+    ext_pattern = r'(\.r\d+c\d+)?\.(png|jpg|pgw|jgw|txt)(.aux.xml)?'


I couldn't convince Owen to go to Black style formatting with the double quotes.

😅

before we used black-jupyter with "skip-string-normalization" which just leaves strings however you put them in

Yup, this.

Why change double quote to single quote?
Consistently?
Coding standards?

I think somewhere in PEP008 there's a sentence about whether to use single or double quotes. The sentiment is basically: it doesn't really matter which, but pick one and use it consistently.

owenlittlejohns · 2024-12-11T23:44:07Z

tests/run_tests.sh

@@ -20,28 +20,19 @@ STATUS=0

 export HDF5_DISABLE_VERSION_CHECK=1


Yeah, this is some straight-up cargo cult stuff from some older version of this script which has been copied and pasted from project to project. Agree with cutting it. (One day this file will be a glorious one liner that just becomes part of the CI/CD directly)

owenlittlejohns · 2024-12-11T23:44:27Z

tests/test_code_format.py

owenlittlejohns · 2024-12-11T23:46:47Z

docker/service.Dockerfile

@@ -15,7 +15,7 @@
 # and updates the entrypoint of the new service container
 #
 ###############################################################################
-FROM python:3.11
+FROM python:3.12


Nice - I'm tempted to suggest releasing a version of HyBIG with just this change of Python version. Feels like it could be a fair candidate for a patch version.

I really don't want to because it's a bunch of overhead for something that will be replaced as soon as I finish all that overhead. The next release will change functionality significantly and require updated regression-tests.

I could release it and not deploy it, but that also seems kind of dumb in that you've just created another version that is lying around.

I just did this in a separate PR to make it easier for someone to review and test. Since this does run against the existing regressions.

Fair enough. Given there isn't a dedicated ticket specifically for this clean up, I can buy that.

owenlittlejohns · 2024-12-11T23:47:07Z

.github/workflows/run_lib_tests.yml

@@ -10,7 +10,7 @@ jobs:
    strategy:
      fail-fast: false
      matrix:
-        python-version: ['3.10', '3.11']
+        python-version: ['3.10', '3.11', '3.12']


owenlittlejohns · 2024-12-11T23:51:03Z

harmony_service/adapter.py

Out of scope for this PR - but if you are cleaning things up, you could tweak the process_item method to use a context manager with tempfile.TemporaryDirectory (like you do in the SMAP L2 Gridder) instead of the mkdtemp and a finally for clean-up. Both work, and both are fine, but I think you convinced me the context manager is marginally simpler.

I like that. Maybe in a follow-on PR.

Ugh, mixing pytest fixtures and unittest patches.

wow that was a dumb 🐰 🕳️ 4fcded7

flamingbear added 8 commits November 22, 2024 11:40

DAS-NONE: Random small fixes.

ea94925

No functional differences.

DAS-NONE: Update pre-commit and pyproject.yaml

b1cf9e8

Removes jupyter-black dependency.

DAS-NONE: Commit changes associated with updated pre-commit file

8304a9c

DAS-NONE: use newer ruff version

192f7e6

DAS-NONE: Update changelog

d094785

DAS-None: Add cause to re-raised exception.

c051954

I didn't know, but the message from the caught exception is not added directly to the raised exception.

DAS-None: change default python version. Clean up test scripts.

b6b05f7

This tweak is going wild.

DAS-None: fix report locations.

920ab4d

flamingbear force-pushed the mhs/DAS-NONE/miscellany branch from 7397a2f to 920ab4d Compare December 10, 2024 23:21

flamingbear requested review from owenlittlejohns and lyonthefrog December 10, 2024 23:28

DAS-None: pycodestyle isn't required anymore

b9bd5c5

flamingbear commented Dec 10, 2024

View reviewed changes

docs/HyBIG-Example-Usage.ipynb Outdated

Copy link

Member Author

flamingbear Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just changed 2 comments for line length.

flamingbear commented Dec 10, 2024

View reviewed changes

DAS-None: Add test for exception message.

edf22bf

flamingbear commented Dec 10, 2024

View reviewed changes

flamingbear marked this pull request as ready for review December 11, 2024 00:13

vutrannasa reviewed Dec 11, 2024

View reviewed changes

DAS-NONE: slim down the message in forbidden test.

8bf518b

Just simplify.

vutrannasa reviewed Dec 11, 2024

View reviewed changes

DAS-NONE: remove doubled code.

38d9f1b

owenlittlejohns approved these changes Dec 11, 2024

View reviewed changes

owenlittlejohns reviewed Dec 11, 2024

View reviewed changes

flamingbear added 2 commits December 11, 2024 17:03

DAS-2276: remove unused flag

54049a8

DAS-NONE: Use TemporaryDirectory context in process_item

4fcded7

flamingbear merged commit 2f47607 into main Dec 12, 2024
7 checks passed

flamingbear deleted the mhs/DAS-NONE/miscellany branch December 12, 2024 16:47

flamingbear mentioned this pull request Dec 12, 2024

DAS-2276: retain 3 and 4 band information in browse images #39

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAS-NONE: Random small fixes. #38

DAS-NONE: Random small fixes. #38

flamingbear commented Dec 10, 2024 •

edited

Loading

flamingbear commented Dec 10, 2024

flamingbear Dec 10, 2024

flamingbear Dec 10, 2024

flamingbear Dec 10, 2024

flamingbear Dec 10, 2024

flamingbear Dec 10, 2024

flamingbear Dec 10, 2024 •

edited

Loading

owenlittlejohns Dec 11, 2024

vutrannasa Dec 11, 2024 •

edited

Loading

flamingbear Dec 11, 2024 •

edited

Loading

owenlittlejohns Dec 11, 2024

vutrannasa Dec 11, 2024

flamingbear Dec 11, 2024

vutrannasa Dec 11, 2024

flamingbear Dec 11, 2024

owenlittlejohns left a comment

owenlittlejohns Dec 11, 2024

owenlittlejohns Dec 11, 2024

owenlittlejohns Dec 11, 2024

owenlittlejohns Dec 11, 2024

flamingbear Dec 12, 2024

owenlittlejohns Dec 12, 2024

owenlittlejohns Dec 11, 2024

owenlittlejohns Dec 11, 2024 •

edited

Loading

flamingbear Dec 12, 2024

flamingbear Dec 12, 2024

flamingbear Dec 12, 2024

		@@ -20,28 +20,19 @@ STATUS=0

		export HDF5_DISABLE_VERSION_CHECK=1

DAS-NONE: Random small fixes. #38

DAS-NONE: Random small fixes. #38

Conversation

flamingbear commented Dec 10, 2024 • edited Loading

Description

Jira Issue ID

Local Test Steps

PR Acceptance Checklist

flamingbear commented Dec 10, 2024

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

flamingbear Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vutrannasa Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

flamingbear Dec 11, 2024 • 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

owenlittlejohns 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenlittlejohns Dec 11, 2024 • 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

flamingbear commented Dec 10, 2024 •

edited

Loading

flamingbear Dec 10, 2024 •

edited

Loading

vutrannasa Dec 11, 2024 •

edited

Loading

flamingbear Dec 11, 2024 •

edited

Loading

owenlittlejohns Dec 11, 2024 •

edited

Loading