Das 2192 #14

sudha-murthy · 2024-08-19T22:00:09Z

Description

Prefetch of dimension variables was occurring for variable subsetting for SMAP L3/L4 collections. The update to the is_index_subset method fixes the bug.

Jira Issue ID

DAS-2192

Local Test Steps. At least one of the urls can be tested. The full test can be done post merge.

Build hoss image locally after fetching the branch DAS-2192
All the following 8 SMAP L3 requests should succeed and the logs should not have a call for 'prefetch'

PR Acceptance Checklist

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

The fix does only corrects variable subsetting. Spatial subsetting will be fixed in DAS-2232 after a fix to varinfo in DAS-2231
The jupyter notebook tests will be updated after the fix to DAS-2231 and DAS-2232 and the corresponding HOSS changes

… SMAP L3/L4 collections

owenlittlejohns

The change makes sense. The two main things are adding a couple of quick subtests to make sure the functionality works as expected and making sure the pre-commit checks are passing. There are instructions here to set up pre-commit for the repository.

owenlittlejohns · 2024-08-19T22:20:29Z

hoss/dimension_utilities.py

@@ -45,7 +45,7 @@ def is_index_subset(message: Message) -> bool:

    """
    return any(
-        rgetattr(message, subset_parameter, None) is not None
+        rgetattr(message, subset_parameter, None) not in (None, [])


I think we need a new unit test (or more likely a sub test) here to make sure the empty list is correctly identified in this check. Probably also a sub test with a non-empty list, too, just to be sure.

Here's the test for this function. I think the easiest thing is to just add two new subtests.

I think there are several subtests with non empty list. But I added 2 subtests for the empty and non empty list cases

Yeah, looking in more detail, I think you are right about the non-empty list. But thanks for adding a couple of unit tests.

owenlittlejohns · 2024-08-19T22:21:51Z

CHANGELOG.md

+## v1.0.5
+### 2024-08-19
+
+This version of HOSS updates the 'is_index_subset' method to check for empty list (in case of dimensions subset) 


Super pedantic nit: The single-quotes around the is_index_subset should probably be backticks so that the function renders as code in the markdown.

owenlittlejohns · 2024-08-19T22:29:19Z

CHANGELOG.md

+### 2024-08-19
+
+This version of HOSS updates the 'is_index_subset' method to check for empty list (in case of dimensions subset) 
+as well as None (for the spatial, bbox and temporal subsets. This prevents prefetch from being called for 


Other nit: This line is missing a closing parenthesis. I think it's meant to be:

as well as None (for spatial, bounding box and temporal subsets). This prevents prefetch from being called for

I guess one other note: I don't know that "prefetch" is too informative for a lot of folks. Maybe:

This prevents 1-D dimension variables from being unnecessarily requested from OPeNDAP for some variable subsets, in particular SMAP L3 and L4 collections.

Nit on line below: "occuring" -> "occurring".

…lists

owenlittlejohns

Thanks for adding the unit test. That looks good.

I think all I have left are the nits from CHANGELOG.md (missing closing parenthesis and adding a bit more context to the prose about the prefetch request to OPeNDAP). Other than that, I found one more nit about the description of the new unit test. Otherwise, this is looking good.

For what it's worth, the reason I'm being a bit pedantic about the CHANGELOG.md is it forms the basis of the public release notes for 1.0.5 release, which are are pretty public-facing, so it's good to make sure there's not typos in them etc.

owenlittlejohns · 2024-08-22T00:33:49Z

tests/unit/test_dimension_utilities.py

@@ -679,6 +679,38 @@ def test_is_index_subset(self):
                )
            )

+        with self.subTest('No index Subset'):


Nitpick: I think it's probably worth explicitly stating this subtest is for an empty dimensions list rather than "No index subset".

owenlittlejohns · 2024-08-22T00:34:20Z

hoss/dimension_utilities.py

@@ -45,7 +45,7 @@ def is_index_subset(message: Message) -> bool:

    """
    return any(
-        rgetattr(message, subset_parameter, None) is not None
+        rgetattr(message, subset_parameter, None) not in (None, [])


Yeah, looking in more detail, I think you are right about the non-empty list. But thanks for adding a couple of unit tests.

autydp

This change + the unit test updates look good and cover the necessary updates. Good work!

sudha-murthy · 2024-08-22T12:56:20Z

This change + the unit test updates look good and cover the necessary updates. Good work!

Thanks @autydp

sudha-murthy · 2024-08-22T13:48:13Z

Thanks for adding the unit test. That looks good.

I think all I have left are the nits from CHANGELOG.md (missing closing parenthesis and adding a bit more context to the prose about the prefetch request to OPeNDAP). Other than that, I found one more nit about the description of the new unit test. Otherwise, this is looking good.

For what it's worth, the reason I'm being a bit pedantic about the CHANGELOG.md is it forms the basis of the public release notes for 1.0.5 release, which are are pretty public-facing, so it's good to make sure there's not typos in them etc.

Thanks @owenlittlejohns
I think I updated them both now. I must have missed committing the CHANGELOG.md before.

owenlittlejohns

Thanks for addressing the comments I had.

When merging please use the "Squash and Merge" option, and be sure to prepend the squashed commit message with the ticket number, so something like "DAS-2192 - Ensure HOSS correctly identifies when an index range subset is necessary". (The commits so far in this PR do not do this)

flamingbear

Looks good, I was following along in the background.

sudha-murthy · 2024-08-22T15:07:16Z

"DAS-2192 - Ensure HOSS correctly identifies when an index range subset is necessary".

sudha-murthy added 2 commits August 19, 2024 17:30

updates to prevent prefetch from being called for variable subset for…

52d977d

… SMAP L3/L4 collections

updates to prevent prefetch from being called for variable subset for…

2428ed6

… SMAP L3/L4 collections

sudha-murthy requested a review from owenlittlejohns August 19, 2024 22:00

sudha-murthy assigned flamingbear Aug 19, 2024

sudha-murthy requested a review from flamingbear August 19, 2024 22:01

sudha-murthy unassigned flamingbear Aug 19, 2024

owenlittlejohns requested changes Aug 19, 2024

View reviewed changes

owenlittlejohns reviewed Aug 19, 2024

View reviewed changes

added couple of unit tests to is_index_subset function to test empty …

424bda3

…lists

sudha-murthy requested a review from lyonthefrog August 20, 2024 13:30

owenlittlejohns reviewed Aug 22, 2024

View reviewed changes

autydp approved these changes Aug 22, 2024

View reviewed changes

updates based on PR feedback

dc7c441

updated CHANGELOG.md again

e4b5270

owenlittlejohns approved these changes Aug 22, 2024

View reviewed changes

flamingbear approved these changes Aug 22, 2024

View reviewed changes

sudha-murthy merged commit 065415e into main Aug 22, 2024
4 checks passed

owenlittlejohns deleted the DAS-2192 branch August 22, 2024 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Das 2192 #14

Das 2192 #14

sudha-murthy commented Aug 19, 2024 •

edited

Loading

owenlittlejohns left a comment

owenlittlejohns Aug 19, 2024

sudha-murthy Aug 20, 2024

owenlittlejohns Aug 22, 2024

owenlittlejohns Aug 19, 2024

owenlittlejohns Aug 19, 2024

owenlittlejohns left a comment •

edited

Loading

owenlittlejohns Aug 22, 2024 •

edited

Loading

owenlittlejohns Aug 22, 2024

autydp left a comment

sudha-murthy commented Aug 22, 2024

sudha-murthy commented Aug 22, 2024

owenlittlejohns left a comment

flamingbear left a comment

sudha-murthy commented Aug 22, 2024

Das 2192 #14

Das 2192 #14

Conversation

sudha-murthy commented Aug 19, 2024 • edited Loading

Description

Jira Issue ID

Local Test Steps. At least one of the urls can be tested. The full test can be done post merge.

PR Acceptance Checklist

owenlittlejohns left a comment

Choose a reason for hiding this comment

owenlittlejohns Aug 19, 2024

Choose a reason for hiding this comment

sudha-murthy Aug 20, 2024

Choose a reason for hiding this comment

owenlittlejohns Aug 22, 2024

Choose a reason for hiding this comment

owenlittlejohns Aug 19, 2024

Choose a reason for hiding this comment

owenlittlejohns Aug 19, 2024

Choose a reason for hiding this comment

owenlittlejohns left a comment • edited Loading

Choose a reason for hiding this comment

owenlittlejohns Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

owenlittlejohns Aug 22, 2024

Choose a reason for hiding this comment

autydp left a comment

Choose a reason for hiding this comment

sudha-murthy commented Aug 22, 2024

sudha-murthy commented Aug 22, 2024

owenlittlejohns left a comment

Choose a reason for hiding this comment

flamingbear left a comment

Choose a reason for hiding this comment

sudha-murthy commented Aug 22, 2024

sudha-murthy commented Aug 19, 2024 •

edited

Loading

owenlittlejohns left a comment •

edited

Loading

owenlittlejohns Aug 22, 2024 •

edited

Loading