Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAS-2256 update earthdata-varinfo version 3.0.0 #25

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

vutrannasa
Copy link
Contributor

Description

Update HOSS (hoss_config.json) to work with earthdata-varinfo version 3.0.0

Jira Issue ID

https://bugs.earthdata.nasa.gov/browse/DAS-2256

Local Test Steps

Local MAC M1

Build harmony-opendap-subsetter

% cd /<your_workspace>
% git clone https://github.com/nasa/harmony-opendap-subsetter DAS-2256-hoss-varinfo-30
% cd DAS-2256-hoss-varinfo-30
% git pull origin DAS-2256-hoss-varinfo-30
% ./bin/build-image
% ./bin/build-test
% ./bin/run-test
% cd ..
% git clone https://git.earthdata.nasa.gov/scm/sitc/das-collections-harmony-testing.git

Start harmony in box if installed on system

% cd /<your_workspace>/harmony
% vim .env (Verify .env has sds-maskfill,hoss)
LOCALLY_DEPLOYED_SERVICES=sds-maskfill,hoss
% bin/bootstrap-harmony

Open /<your_workspace>/das-collections-harmony-testing.folder in VSCode and run below tests using Jupiter notebook
Python Environment: hoss_tests

hoss/HOSS_SPL2SMAP_S_tests.ipynb
hoss/HOSS_SPL3SMA_tests.ipynb
hoss/HOSS_SPL3SMAP_tests.ipynb
hoss/HOSS_SPL3SMP_E_tests.ipynb
hoss/HOSS_SPL3SMP_tests.ipynb

Verify above tests all PASS

1. variable_subset_request_status :  Success
2. bbox_subset_request_status :  Success
3. shape_file_subset_request_status :  Success

PR Acceptance Checklist

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

@@ -265,19 +227,19 @@
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section does not need to be updated. The attributes can remain as float 0.0 and 90.0
The examples in https://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/build/apf.html are in float
The changes are unrelated to varinfo update.

},
{
"Applicability": {
"Mission": "MERRA-2",
"Variable_Pattern": "/lat"
"VariablePattern": "/lat"
},
"Attributes": [
{
"Name": "valid_range",
"Value": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this needs to be updated either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of migrating to earthdata-varinfo 3.0.0 is to
Removing underscores in JSON schema attribute names.

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

The changes look good. I'm having a couple of issues with my local Harmony instance, so I was not able to test these changes out locally using the notebooks you mentioned (although those are great test instructions).

I did run the new version of hoss_config.json through jsonschema to ensure that it was compliant with the schema definition, which it was. Nice.

I also locally built the service and test image to confirm that the unit tests worked as expected (I had to add --platform linux/amd64 to the docker build and docker run commands, so that might be a change that should be made in a future PR)

@@ -1,6 +1,6 @@
# This file should contain requirements to be installed via Pip.
# Open source packages available from PyPI
earthdata-varinfo ~= 2.3.0
earthdata-varinfo ~= 3.0.0
Copy link
Member

Choose a reason for hiding this comment

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

(Not a blocker) Might as well bump this directly to 3.0.1, which is the latest version. (The difference between 3.0.0 and 3.0.1 won't have any impact on this PR, but the ~= operator will resolve the dependency version to 3.0.1 anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update to 3.0.1

@@ -690,192 +661,203 @@
{
"Applicability": {
"Mission": "GEDI",
"ShortNamePath": "GEDI_L[1234][AB]|GEDI0[1234]_[AB]"
"ShortNamePath": "GEDI_L[1234][AB]|GEDI0[1234]_[AB]",
"VariablePattern": "/$"
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch - this was a tricky bit that took a bit more care when converting over the other services.

@sudha-murthy
Copy link
Contributor

I was able to run all the notebooks and they all passed

Copy link
Contributor

@joeyschultz joeyschultz left a comment

Choose a reason for hiding this comment

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

Unit tests all passed. Having some harmony-in-a-box issues that are preventing me from running all the notebooks.

CHANGELOG.md Outdated
## v1.1.2
### 2025-01-20

- [[DAS-2287](https://bugs.earthdata.nasa.gov/browse/DAS-2287)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the text and link to DAS-2256 instead of DAS-2287

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@vutrannasa vutrannasa merged commit cc433c8 into main Jan 23, 2025
4 checks passed
@vutrannasa vutrannasa deleted the DAS-2256-hoss-varinfo-30 branch January 23, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants