search

payu-org / payu

A workflow management tool for numerical models on the NCI computing systems
Apache License 2.0
18 stars 25 forks source link

Update to packaging tools and logic #403

Closed dougiesquire closed 5 months ago

dougiesquire commented 5 months ago

This PR implements some changes discussed in #398:

In doing this, I discovered that the um driver requires imp, which has been deprecated. I’ve updated the driver to use the replacement importlib instead and the tests still pass, but I'm not sure the tests actually touch the modified code...

I've also removed the tools and bin directories (#396 and #397). This doesn't have to be done here, but it felt relevant since these are places where the hard-coded version was being used.

~I couldn’t work out a easy/tidy way to have dependencies defined in only one location for the PyPI and conda builds, so dependencies are currently listed in both pyproject.toml and meta.yaml~. NO LONGER TRUE - the dependencies and entry points in meta.yaml are input dynamically from pyproject.toml.

~I’ve tested building the PyPI dist, uploading to TestPyPI and then doing the conda build locally from that. But the true test will be actually issuing a new release.~

Closes #398, closes #401, closes #397, closes #396

pep8speaks commented 5 months ago

Hello @dougiesquire! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 381:80: E501 line too long (84 > 79 characters) Line 471:80: E501 line too long (80 > 79 characters) Line 474:80: E501 line too long (84 > 79 characters)

Comment last updated at 2024-01-22 04:26:34 UTC
dougiesquire commented 5 months ago

The conda-build job that's been added to CI.yaml will need to be altered or removed. Is it necessary anymore?

coveralls commented 5 months ago

Coverage Status

coverage: 51.69% (-0.9%) from 52.591% when pulling e094ac419ba3045b3062b4bc9a3681ec40ee8278 on dougiesquire:issue-398 into ca0c825aaa1b228f1350f32b2318f40f29760aef on payu-org:master.

CodeGat commented 5 months ago

Hey @dougiesquire! With regards to https://github.com/payu-org/payu/pull/403#issuecomment-1894921909 I think we'd need the equivalent check for PyPI, in this case. We used the conda build check to make sure we weren't putting anything broken onto vk83 (which had happened before). Is there a similar thing where you can attempt to build Payu using PyPI?

dougiesquire commented 5 months ago

I think we'd need the equivalent check for PyPI, in this case. We used the conda build check to make sure we weren't putting anything broken onto vk83 (which had happened before). Is there a similar thing where you can attempt to build Payu using PyPI?

Sure. The current build job in the CI is really more about running the tests. So we could rename this to test and add a build job to build the distribution with PyPA's build package? Of course, success of that job still won't guarantee a successful conda build.

We could do something like upload the dist resulting from the PyPA build to TestPyPI and then try and build the conda build from that, but I'm not sure how the tag-based versioning would work...

dougiesquire commented 5 months ago

I've worked out a way to dynamically input the dependencies in meta.yaml from the pyproject.toml. However, it requires that the dependencies are listed in the pyproject.toml using the slightly more restricted syntax expected by conda (i.e. a space before version limits, but not after, e.g. package >=1.0). It also obviously will fail if the conda dependencies differ from the pip dependencies. @aidanheerdegen do we want to do this? I can add a comment to the pyproject.toml about the syntax. ADDED: see this commit.

Regarding the build tests in the CI, I see three options:

  1. only build with PyPA's build package. Passing this isn't necessarily an indication that a release will be successful as the upload to PyPI or the conda build from PyPI could have untested issues.
  2. Build with PyPA's build package and conda build from local source. These can be independent jobs. This will require some jinja templating in the meta.yaml to enable the source to be switched dynamically (e.g. based on an env variable set in the Github workflow) since we will want to build from local source in CI and PyPI in CD.
  3. Build with PyPA's build package, upload to TestPyPI and conda build from TestPyPI dist. These will be dependent jobs. Again jinja templating will be required to switch the PyPI url depending on whether CI (https://test.pypi.org) or CD (https://pypi.io).

Curious on thoughts @CodeGat, @aidanheerdegen. 1 or 3 are probably my most preferred options. ADDED: e.g. 3 might look something like this commit.

dougiesquire commented 5 months ago

I can't work out why the trusted publishing to TestPyPI is not working (it says here that it's supported). I'm going to stop working on this and give @aidanheerdegen and @CodeGat some time to respond to my questions above. And if anyone has ideas about why trusted publishing isn't working, please let me know!

aidanheerdegen commented 5 months ago

I've worked out a way to dynamically input the dependencies in meta.yaml from the pyproject.toml. However, it requires that the dependencies are listed in the pyproject.toml using the slightly more restricted syntax expected by conda (i.e. a space before version limits, but not after, e.g. package >=1.0).

This looks like a great approach. Jinja templating is a standard feature of conda/meta.yaml so it isn't going to mysteriously stop working. The requirement to match the stricter syntax for conda version pinning is a feature IMO. We already had an issue with this when duplicating requirements from setup.py to meta.yaml. Being explicit and doing it one place is much better. Just need to make sure we test for correctness in the CI (by test deployment, or linting if that is an available option).

Regarding the build tests in the CI, I see three options

One clear goal is that the PyPI and conda distributions are the same/consistent/identical, and that there is minimal overhead in maintaining that.

One route to making the deployment identical is to deploy to PyPI and then deploy conda from PyPI which is what condo-forge does.

We’re running our CD for PyPI and conda from the same repo so we can orchestrate it to build the package locally and deploy to PyPI and then build conda from the same local version of the built package directly, rather than downloading it from PyPI. Right? That corresponds roughly to your option 2 I believe?

I can't see a lot of downside to that.

dougiesquire commented 5 months ago

One route to making the deployment identical is to deploy to PyPI and then deploy conda from PyPI which is what condo-forge does.

This is what I've currently set up in CD.yml and it should work for releases. But we wanted to also add test builds to CI.yml to catch issues early. Obviously we don't want to upload to PyPI as part of this (because that would be a release), so I've tried to upload to TestPyPI. Unfortunately the trusted publishing is failing when trying to upload to TestPyPI. I don't understand why - others are doing this successfully, e.g. xarray.

I can't see a lot of downside to that.

Yeah, I don't think there's a downside. I liked the idea of also testing the upload using TestPyPI and got fixated on why it's not working. I'll try creating a new PyPI account and if that doesn't fix the issue I'll give up.

dougiesquire commented 5 months ago

Okay, I gave up on trying to test the upload via TestPyPI. The conda build is now being done from local source, rather than from the PyPI build. So not that much has actually changed in the end. I'll edit the original description to be accurate about what's changed and then I think I'll leave it there.

Sorry for the messy commit history. I took the "suck it and see" approach to debugging ci workflows...

dougiesquire commented 5 months ago

Ugh sorry, did my OCD changing of the title dismiss your reviews @aidanheerdegen and @jo-basevi?

aidanheerdegen commented 5 months ago

Looks like it eh?