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

Add Github CI/CD workflows #19

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jo-basevi
Copy link
Collaborator

@jo-basevi jo-basevi commented Dec 3, 2024

This PR adds CI workflows for building a conda package, and runs tests, and CD workflow that will publish to access-nri conda channel. The CI/CD workflows are basically copied from um2nc-standalone repository (https://github.com/ACCESS-NRI/um2nc-standalone). I've added a pyproject file, and versioneer for dynamic versioning so new packages will be deployed when a new git tag is pushed. I've tested it the CI workflows pass on separate repository, though the Filter files step did fail for me but I think that was just because the test repository I was using was private so there were permission errors.

TODO:

  • secrets.ANACONDA_TOKEN will need to be added to this repository

Closes #17
Closes #15

@jo-basevi jo-basevi marked this pull request as ready for review December 3, 2024 08:26
@CodeGat CodeGat self-requested a review December 3, 2024 21:22
Copy link
Contributor

@CodeGat CodeGat left a comment

Choose a reason for hiding this comment

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

Thanks Jo, looks good. I can't wait to be able to have a conda-package repo that strips out all this common logic (like build-cd/model-config-tests)

channels:
- accessnri
- conda-forge
- coecms
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, when do we get rid of the coecms dependency, given it's shut down now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a dependency on nchash (https://github.com/aidanheerdegen/nchash) which is deployed to the coecms conda channel. Though I'm unsure on whether the dependency is still required - see this existing issue: #11.

uses: conda-incubator/setup-miniconda@a4260408e20b96e80095f42ff7f1a15b27dd94ca # v3.0.4
with:
miniconda-version: "latest"
python-version: "3.10" # TODO: Add back in vars.PY_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be changed back to vars.PY_VERSION?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I had it commented out I think so I could test the CI without modifying the repository settings but it is now changed back in d8cae0e

.github/workflows/CI.yml Outdated Show resolved Hide resolved
- name: Run conda build
shell: bash -el {0}
run: |
conda build . --no-anaconda-upload --output-folder=./build -c conda-forge -c accessnri -c coecms
Copy link
Contributor

Choose a reason for hiding this comment

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

In the channel list we have access-nri > conda-forge > coecms - does that need to be replicated here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I've removed the conda channels from the command (see d8cae0e) and it seems to not require them as the test runs without errors. So I've left them out to hopefully prevent any inconsistencies with the channel list in the environment file.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
Comment on lines 101 to 103
# - name: Lint # TODO: Add back in linting
# shell: bash -l {0}
# run: pylint --extension-pkg-whitelist=netCDF4 --ignored-modules=yamanifest -E yamanifest
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be added back in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’ve just removed the pylint dependency as it was what was removed recently in um2nc-standalone repository which I used as a reference for the conda deployment CI/CD (ACCESS-NRI/um2nc-standalone#167). It would be good to set up a pre-commit hook for linting at some point (if organisational wide linting CI workflows are not set up), but I think that should be a separate PR

Add in conda-verify ignore descriptions

Co-authored-by: Tommy Gatti <tommy.gatti@anu.edu.au>
@jo-basevi jo-basevi marked this pull request as draft December 10, 2024 00:45
- Use Github variable for python version in conda build
- Remove channels from conda build
- Use --exit in conda-verify so the step exits when a check fails
@jo-basevi jo-basevi force-pushed the 17-Add-CD-conda-deployment-to-access-nri branch from 8bab942 to d7ae55d Compare December 10, 2024 02:01
@jo-basevi
Copy link
Collaborator Author

I can't wait to be able to have a conda-package repo that strips out all this common logic (like build-cd/model-config-tests)

Yes that would be so good!

@jo-basevi jo-basevi marked this pull request as ready for review December 10, 2024 02:13
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.

Change CD to GitHub workflow and deploy to accessnri conda channel Add tests to CI
2 participants