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

Changes to support OM3 development branches in QA checks #103

Merged
merged 6 commits into from
Feb 19, 2025

Conversation

anton-seaice
Copy link
Contributor

Contibrutes to #99

This change splits the QA tests (the ones run on a github runner which don't run the model) into two sections. There is a section for Release branches and a section for Dev branches in configurations.

The dev branches tests are triggered via a new dev_config pytest marker. The behaviour of the existing config marker is unchanged, in that it runs both the tests for Release branches and those for Dev branches. The Dev branches test can be considered a subset of the tests applied for Release branches.

For consistency with other models , a test was added that the executable location is the spack release location.

@aidanheerdegen - mostly tagged you for a view on which tests should be dev vs release

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.77%. Comparing base (750ecc3) to head (eed3eff).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...rc/model_config_tests/qa/test_access_om3_config.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   72.56%   72.77%   +0.21%     
==========================================
  Files          16       17       +1     
  Lines         882      889       +7     
==========================================
+ Hits          640      647       +7     
  Misses        242      242              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aidanheerdegen
Copy link
Member

Tests I think you definitely do want in dev_config:

test_sync_is_not_enabled
test_sync_path_is_not_set
test_experiment_name_is_not_defined
test_metadata_does_contain_UUID (should rename that to test_metadata_does_not_contain_UUID for clarity)

You don't want a shared experiment with a sync dir enabled. If you set the experiment name branching doesn't work and you definitely don't want the UUID defined in the metadata.yaml for a configuration. Not the case for an experiment though.

Copy link
Collaborator

@jo-basevi jo-basevi left a comment

Choose a reason for hiding this comment

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

Thanks for adding this in @anton-seaice!

Will the access_om3 marker being used for the dev branches? If so, just double checking if that you are expecting dev-* branches to use a released model modules, e.g.:

modules:
    use:
        - /g/data/vk83/modules
    load:
        - access-om3/<version>

As the test_access_om3_manifest_exe_in_release_spack_location will check for a release Github artefact for a spack.location file. So this will fail for any prerelease modules.

@anton-seaice
Copy link
Contributor Author

If so, just double checking if that you are expecting dev-* branches to use a released model modules

Yes - this is correct. We consider them alpha releases but we do deploy them as pre-release is too short lived.

@anton-seaice
Copy link
Contributor Author

Thanks @aidanheerdegen and @jo-basevi - i've addressed those comments

jo-basevi
jo-basevi previously approved these changes Feb 19, 2025
Copy link
Collaborator

@jo-basevi jo-basevi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

aidanheerdegen
aidanheerdegen previously approved these changes Feb 19, 2025
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

dougiesquire
dougiesquire previously approved these changes Feb 19, 2025
Copy link
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

Not really needed at this point, but I did look and LGTM too

@anton-seaice
Copy link
Contributor Author

Can someone approve again please ? 😂

@anton-seaice anton-seaice merged commit fe4f2e7 into main Feb 19, 2025
8 checks passed
@anton-seaice anton-seaice deleted the 99-om3_dev_branches branch February 19, 2025 22:06
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