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

Harmony 571 daac regression tests #2

Merged
merged 19 commits into from
Mar 8, 2021

Conversation

asteiker
Copy link
Contributor

@asteiker asteiker commented Mar 2, 2021

Updated the regression notebook in the Harmony repo (HarmonyRegression.ipynb) and copied into /test/harmony-regression. Added necessary harmony_host_url variable and modified each test to ensure compatibility across providers. Also added a few lines to test intake-stac compatibility of zarr outputs but left direct access of outputs commented out due to known issues with stac (intake/intake-stac#48). A few assumptions:

  1. I cannot test in SIT but I believe SIT points to UAT data so the collections that are tested are the same across SIT and UAT.
  2. I was able to build and test the environment.yaml locally but did not do a final check using the local docker instructions in the Readme (though I had tested this successfully with an intermediate notebook draft.
  3. I am not able to test in aws due to ngap issues with my account when ssm'ing but James had successfully tested with an intermediate notebook draft.

This branch also has several updates and fixes that James completed including updates to the notebook helper functions under /test/harmony-regression to get some of the plotting working in the docker environment, and fixed a timeout issue to enable aws testing.

Copy link

@indiejames indiejames left a comment

Choose a reason for hiding this comment

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

Tested locally, in SIT, and UAT.

test/Dockerfile Outdated
COPY .netrc .netrc
RUN mkdir ./${sub_dir}
COPY ${sub_dir}/conda-linux-64.lock ./${sub_dir}
RUN ls ${sub_dir}
Copy link
Member

Choose a reason for hiding this comment

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

Debugging statement. Should be removed.

Choose a reason for hiding this comment

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

done

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
script/test.sh Show resolved Hide resolved
sshconfig Show resolved Hide resolved
@@ -0,0 +1,377 @@
from contextlib import contextmanager
Copy link
Member

Choose a reason for hiding this comment

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

There are possibly parts of this we could trim down or exclude, based on not caring much about the visual output in the current notebooks. I'd vote for leaving it as is for now but revisiting once we revamp the existing notebooks to use the newly-developed Python lib.

Copy link
Member

Choose a reason for hiding this comment

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

Doing the trimming down could remove a number of hefty dependencies in the current code

Copy link
Member

Choose a reason for hiding this comment

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

HARMONY-762

test/harmony-regression/notebook_helpers/requirements.txt Outdated Show resolved Hide resolved
test/run_notebooks.sh Show resolved Hide resolved
@bilts bilts self-requested a review March 8, 2021 14:21
Copy link
Member

@bilts bilts left a comment

Choose a reason for hiding this comment

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

The latest updates address everything I had.

@asteiker asteiker merged commit 960d939 into main Mar 8, 2021
@asteiker asteiker deleted the harmony-571-daac-regression-tests branch March 8, 2021 20:43
flamingbear added a commit that referenced this pull request Jan 16, 2023
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.

3 participants