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-2254: wrap gridding code with harmony_service #3

Merged

Conversation

flamingbear
Copy link
Member

@flamingbear flamingbear commented Nov 26, 2024

Description

This PR adds the code and infrastructure to integrate the regridding capabilities into a Harmony Service call.

The Ticket AC:

  • Service Image is built in Docker
  • The SMAP-L2-Gridding-Service can be invoked by a HarmonyAdapter.invoke() call with a correct Message and the output is correct.
  • Service can be run in Harmony-In-A-Box

Jira Issue ID

DAS-2254

Local Test Steps

  • Pull this branch
  • Build and run the tests.
❯ ./bin/build-image  && ./bin/build-test && ./bin/run-test
  • Ensure the tests all pass
================== 19 passed, 1 skipped, 7 warnings in 0.95s ===================

1 skipped This is because creating a datatree (only in pytest fixtures) is incredibly slow.

Test service in Harmony-In-A-Box.

LOCALLY_DEPLOYED_SERVICES=<[blah,blah,etc]>,smap-l2-gridder
SMAP_L2_GRIDDER_COLLECTIONS=C1263071064-NSIDC_CUAT
  • These are included if you are building Harmony from scratch with nvm use, npm i, npm run build, and npm run build-all:
    (I recommend this since it will build native docker images on an ARM machine rather than running under rosetta translation which has been problematic)
HARMONY_IMAGE=harmonyservices/harmony:latest
QUERY_CMR_IMAGE=harmonyservices/query-cmr:latest
QUERY_CMR_SERVICE_QUEUE_URLS='["harmonyservices/query-cmr:latest,http://sqs.us-west-2.localhost.localstack.cloud:4566/000000000000/query-cmr.fifo"]'
SERVICE_RUNNER_IMAGE=harmonyservices/service-runner:latest
WORK_ITEM_SCHEDULER_IMAGE=harmonyservices/work-scheduler:latest
WORK_ITEM_UPDATER_IMAGE=harmonyservices/work-updater:latest
WORK_REAPER_IMAGE=harmonyservices/work-reaper:latest
WORK_FAILER_IMAGE=harmonyservices/work-failer:latest
  • Hop on the VPN and drop this url in your browser to kick off a test of the service:
http://localhost:3000/C1263071064-NSIDC_CUAT/ogc-api-coverages/1.0.0/collections/all/coverage/rangeset?forceAsync=true&outputcrs=EPSG%3A4326&format=application%2Fx-netcdf4&maxResults=1
  • Open your workflow-ui and see that it is running the smap-l2-gridder.

  • Visit the job page and download the asset SMAP_L2_SM_P_E_46849_A_20231108T210834_R19240_003_regridded.nc

  • Open the file and see that it looks and behaves as expected with xarray or panoply.

PR Acceptance Checklist

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

Leaving docker/service_version.txt at 0.0.0 until the next ticket is complete, which is to Use GitHub Actions to Build and Test the SMAP-L2-Gridder (DAS-2267)

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is shorter than some of the other service run_tests.sh scripts. It's still doing all of the things. L13-15 runs the tests and generates the coverage report and sets status to 1 if it fails.

I still like running the pylint even if ruff pre-commit handles a lot, it has some other information that can be gleaned with respect to best practices.

Copy link
Member

Choose a reason for hiding this comment

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

Ha - I missed this comment while I was reviewing and left a related comment. Perhaps the better way to go is to use a pylint plugin for pytest? That way we could strip this down to a single command.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do it, but the output is actually different. 🙄

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

'EASE_row_index',
'albedo',
'crs',
#'tb_time_utc',
Copy link
Member Author

Choose a reason for hiding this comment

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

These are commented out because of changing the pytest fixture for the input datatree.

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.

Nice work! I don't think I had any massive suggestions here, just a couple of questions and a couple of minor nits.

.github/workflows/mypy.yml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
bin/build-image Outdated Show resolved Hide resolved
tests/test_service/test_adapter.py Show resolved Hide resolved
)

# Invoke the adapter.
_, _ = smap_l2_gridding_service.invoke()
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure it makes sense to drop the outputs from HarmonyAdapter.invoke, but just want to ask out loud if we should be looking at them and making assertions.

Copy link
Member Author

@flamingbear flamingbear Nov 27, 2024

Choose a reason for hiding this comment

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

I kind of thought they were useless but they can be added.

I checked, and there's nothing of interest in the output stac catalog because everything is mocked beyond. The interesting stuff is that we actually call the gridding logic and can open the file. That said, I mocked the Asset call to add one more check.

download_mock.assert_called_once_with(
asset_href,
tmp_path,
logger=mocker.ANY,
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to get more specific here. I'm not 100% sure, but you might be able to use isinstance(Logger) (assuming you've got from logging import Logger up above).

With the cfg below - could you actually assert against the object itself? I was wondering if instead of just declaring the config=config(validate=False) when you instantiate the SMAPL2GridderAdapter, you could separately declare that and reuse it here for completeness.

(Neither of these things are massive, but generally I get wary when I see assertions against ANY)

Copy link
Member Author

Choose a reason for hiding this comment

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

Show me how the logger instance works and I'll do it, but I can't figure it out.

For the config: 66e5620


# Fixtures
@pytest.fixture
def sample_datatree_file(tmp_path) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this because you are putting things in a temporary directory, but you can also use a yield in a pytest fixture, instead of a return. The stuff after the yield is executed similar to a teardown method in a unittest.TestCase class. I was thinking about it here, because you could delete the file after the yield statement. But you don't need to here if you're just dumping files into a temporary directory that cleans itself up.

(Sorry if I'm preaching things at you that you already know)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's good info, I think that popped into my head when I was looking at examples, but never went back to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

what are you thinking wrap the whole thing in it's own temp dir? and yield it?

Copy link
Member

Choose a reason for hiding this comment

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

I think these tests look pretty thorough. Because there are no format options in the message that are needed, it's not like there are a bunch of things to test out working with/without each other.

prepend `harmony-` to the service name to be consistent with other services.
Because that's what it is apparently.
use a config and verify it was used in a mock.
Mock Asset() call
@flamingbear
Copy link
Member Author

@owenlittlejohns I'm merging, but will address anything in the next ticket if you have further comments.

@flamingbear flamingbear merged commit f52ec3a into nasa:main Nov 27, 2024
2 checks passed
@flamingbear flamingbear deleted the mhs/DAS-2254/integrate-as-harmony-service branch November 27, 2024 23:13
flamingbear added a commit to flamingbear/harmony-SMAP-L2-gridding-service that referenced this pull request Nov 30, 2024
This was from a previous PR that I didn't address at the time.

nasa#3 (comment)
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.

2 participants