-
Notifications
You must be signed in to change notification settings - Fork 1
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
DAS-2254: wrap gridding code with harmony_service #3
Conversation
This also lets a user run this from the commandline with or without docker and the results go to the same place.
That really didn't make sense, but the test time went from 7+ to 1.1 sec with this change.
- Adds ruff config to pyproject.toml
Just adding instructions.
Just ran `ruff format .` with the single-quote config on pyproject.toml.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
tests/test_service/test_adapter.py
Outdated
) | ||
|
||
# Invoke the adapter. | ||
_, _ = smap_l2_gridding_service.invoke() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@owenlittlejohns I'm merging, but will address anything in the next ticket if you have further comments. |
This was from a previous PR that I didn't address at the time. nasa#3 (comment)
Description
This PR adds the code and infrastructure to integrate the regridding capabilities into a Harmony Service call.
The Ticket AC:
Jira Issue ID
DAS-2254
Local Test Steps
1 skipped This is because creating a datatree (only in pytest fixtures) is incredibly slow.
Test service in Harmony-In-A-Box.
To test the service, you will have to use a branch from the flamingbear fork of harmony.
https://github.com/flamingbear/harmony/tree/mhs/DAS-2254/smap-l2-gridder-service-addition
This contains most of the configuration and environment needed to run harmony with the new service.
You don't have to build from scratch, but I recommend it.
Set some harmony environmental variables:
nvm use
,npm i
,npm run build
, andnpm 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)
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
CHANGELOG.md
updated to include high level summary of PR changes.docker/service_version.txt
updated if publishing a release.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)