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 conda packaging and deployment to accessnri channel #74

Merged
merged 15 commits into from
Aug 20, 2024

Conversation

aidanheerdegen
Copy link
Member

@aidanheerdegen aidanheerdegen commented Aug 15, 2024

Fixes #71

I pretty much just copied what @dougiesquire had done on the payu repo.

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.

Changes look good to me, just a couple of small things. It could be good to add a CI to test the conda build step? (e.g. in payu: https://github.com/payu-org/payu/blob/e611bcca30a95be96903821d8a083baf2c8cdde7/.github/workflows/CI.yml#L38-L56)

.github/workflows/CD.yml Outdated Show resolved Hide resolved
conda/meta.yml Outdated Show resolved Hide resolved
Copy link

@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.

Mostly just the target branch of this pull request.

.github/workflows/CD.yml Outdated Show resolved Hide resolved
.pylintrc Show resolved Hide resolved
conda/meta.yml Outdated Show resolved Hide resolved
@aidanheerdegen
Copy link
Member Author

I could change the target to main, or keep on develop and merge develop with main ASAP.

@aidanheerdegen aidanheerdegen dismissed CodeGat’s stale review August 15, 2024 05:07

Change is no longer relevant. Not using branch names in CD.

@aidanheerdegen
Copy link
Member Author

Seems the CI interprets the python version as a number, so "3.10" becomes 3.1.

https://github.com/ACCESS-NRI/um2nc-standalone/actions/runs/10399122598/job/28797544390?pr=74#step:3:4

I've quote the strings, and they seem to work ok in the intake catalogue CI, which looks the same to my eye

https://github.com/ACCESS-NRI/access-nri-intake-catalog/actions/runs/10295656976/job/28495683756#step:3:4

Any suggestions/help would be welcome.

CodeGat
CodeGat previously approved these changes Aug 15, 2024
Copy link

@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.

Yeah gotta quote those strings! That should be fine as is

@CodeGat
Copy link

CodeGat commented Aug 15, 2024

I'm not sure I understand the failing tests, though...

@truth-quark
Copy link
Collaborator

The primary dev branch is temporarily set to develop while there's a lot of change activity.

@jo-basevi
Copy link
Collaborator

I am not sure why the conda build is failing, my initial guess was there's no git tag versions for versioneer so the meta.yaml isn't valid but I think versioneer might give a default? I wonder if it's worth adding a --debug flag to the conda build cmd conda build . --no-anaconda-upload and hope it gives some more information?

@jo-basevi
Copy link
Collaborator

In payu, the filename is meta.yaml vs meta.yml? The conda docs use meta.yaml - https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html. This stack-overflow issue run into issues with conda build ignoring the meta.yml file (https://stackoverflow.com/questions/73405205/conda-build-ignoring-meta-yml). I don't know if that would make a difference

@aidanheerdegen
Copy link
Member Author

In payu, the filename is meta.yaml vs meta.yml?

Heh. Yeah, thanks @jo-basevi. I was sure I'd copied that from somewhere else, but now I'm not so sure. meta.yml seems to work in some cases. conda certainly complained when I had both files present, which is kinda weird.

So yes that was the proximate error of not being able to find a recipe file.

@aidanheerdegen
Copy link
Member Author

I am not sure why the conda build is failing, my initial guess was there's no git tag versions for versioneer so the meta.yaml isn't valid but I think versioneer might give a default? I wonder if it's worth adding a --debug flag to the conda build cmd conda build . --no-anaconda-upload and hope it gives some more information?

Again most excellent advice. I was testing locally with those commands, and yeah I wonder if we should just add the --debug flag so there is some more verbose output if something goes awry.

@aidanheerdegen
Copy link
Member Author

Now the CI tests complain because um2nc isn’t pip install-able without the mule conda dependency being installed in the environment.

An elegant solution is to save the package from the conda-build stage as an artefact and conda install it in the test conda env, which will pull down the conda package dependencies.

@CodeGat can you point me to an example I can shamelessly copy to achieve this?

@aidanheerdegen
Copy link
Member Author

So all the infra stuff is working, now failing on the tests

https://github.com/ACCESS-NRI/um2nc-standalone/actions/runs/10414392571/job/28843293751?pr=74#step:8:12

Will consult with @truth-quark, but am considering having allowed-fails for these and fix them later when the interface if stabilised.

@aidanheerdegen
Copy link
Member Author

There was an import of um2netcdf4 that is left over crufty transition to new infra. This was fixed/avoided in #57 so I rebased and force-pushed.

@CodeGat
Copy link

CodeGat commented Aug 16, 2024

SUCCESS!!!!!!!! 🥳

@aidanheerdegen
Copy link
Member Author

Well hello there passing tests, looking pretty good in your slinky green ticks ...

CodeGat
CodeGat previously approved these changes Aug 16, 2024
Copy link

@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.

Excellent stuff

@aidanheerdegen
Copy link
Member Author

@truth-quark is going to take a look on Monday, so will wait on that for merging.

Change CI to deploy on tagging.

Change to modern pytest invocation.

Added in change to license in conda metadata
Quote python version string
Add pytest and pytest-cov to test environment
@aidanheerdegen
Copy link
Member Author

I cleaned up commit history and force-pushed ready for merging on Monday.

@truth-quark
Copy link
Collaborator

I cleaned up commit history and force-pushed ready for merging on Monday.

I'm not a CI specialist, but will review what I can...

truth-quark
truth-quark previously approved these changes Aug 19, 2024
Copy link
Collaborator

@truth-quark truth-quark left a comment

Choose a reason for hiding this comment

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

I've looked over the config & only found a few minor things. Otherwise, nothing stands out given I'm not a CI/CD expert. It's possible some more nuanced issues could be overlooked...

@aidanheerdegen
Copy link
Member Author

@truth-quark can you check you're happy with my responses to your queries. If you're satisfied please mark the unresolved conversations as resolved and if you could provide another approval as I've pushed some changes based on your review.

Thanks!

CodeGat
CodeGat previously approved these changes Aug 19, 2024
.github/workflows/CI.yml Outdated Show resolved Hide resolved
@truth-quark
Copy link
Collaborator

@truth-quark can you check you're happy with my responses to your queries. If you're satisfied please mark the unresolved conversations as resolved and if you could provide another approval as I've pushed some changes based on your review.

Good to go! The only outstanding conversation is @CodeGat's comment on the debug section.

@aidanheerdegen
Copy link
Member Author

If we want nice entry points, e.g. the ability to just call um2nc from the command line, then we'll need main or similar functions defined, as entry points need to point at a function.

https://packaging.python.org/en/latest/specifications/pyproject-toml/#entry-points

This looks like this in the pyproject.toml script:

[project.scripts]
um2nc = "umpost.um2netcdf:main"

When I try and omit the function specifier

[project.scripts]
um2nc = "umpost.um2netcdf"

it throws this error:

ERROR: For req: um2nc==0+untagged.179.g0e246cc.dirty. Invalid script entry point: <ExportEntry um2nc = umpost.um2netcdf:None []> - A callable suffix is required. Cf http
s://packaging.python.org/specifications/entry-points/#use-for-scripts for more information.

I've tested __main__ as the target function, but that also doesn't work with the current set up.

I'm going to remove entry points for this PR and they can be added back in a separate issue when appropriate functions are available as targets.

@aidanheerdegen aidanheerdegen merged commit eb574f3 into develop Aug 20, 2024
4 checks passed
This was referenced Aug 22, 2024
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.

Add conda packaging and deployment to accessnri channel
4 participants