-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
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.
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)
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.
Mostly just the target branch of this pull request.
ec260d9
to
b61df5b
Compare
I could change the target to |
Change is no longer relevant. Not using branch names in CD.
Seems the CI interprets the python version as a number, so "3.10" becomes 3.1. I've quote the strings, and they seem to work ok in the intake catalogue CI, which looks the same to my eye Any suggestions/help would be welcome. |
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.
Yeah gotta quote those strings! That should be fine as is
I'm not sure I understand the failing tests, though... |
The primary dev branch is temporarily set to |
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 |
In payu, the filename is |
Heh. Yeah, thanks @jo-basevi. I was sure I'd copied that from somewhere else, but now I'm not so sure. So yes that was the proximate error of not being able to find a recipe file. |
Again most excellent advice. I was testing locally with those commands, and yeah I wonder if we should just add the |
Now the CI tests complain because An elegant solution is to save the package from the @CodeGat can you point me to an example I can shamelessly copy to achieve this? |
So all the infra stuff is working, now failing on the tests Will consult with @truth-quark, but am considering having allowed-fails for these and fix them later when the interface if stabilised. |
29f5371
to
01d71db
Compare
There was an import of |
SUCCESS!!!!!!!! 🥳 |
Well hello there passing tests, looking pretty good in your slinky green ticks ... |
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.
Excellent stuff
@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
ce967bb
to
e998cce
Compare
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... |
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'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...
0e5907b
@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! |
Good to go! The only outstanding conversation is @CodeGat's comment on the debug section. |
If we want nice entry points, e.g. the ability to just call https://packaging.python.org/en/latest/specifications/pyproject-toml/#entry-points This looks like this in the [project.scripts]
um2nc = "umpost.um2netcdf:main" When I try and omit the function specifier [project.scripts]
um2nc = "umpost.um2netcdf" it throws this error:
I've tested 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. |
Fixes #71
I pretty much just copied what @dougiesquire had done on the payu repo.