MDAnalysis / cookiecutter-mdakit

Cookiecutter for Python packages based on MDAnalysis
MIT License
7 stars 5 forks source link

[CookieCutter CI] Decouple "latest" and "develop" MDAnalysis compatibility checks #110

Open IAlibay opened 8 months ago

IAlibay commented 8 months ago

Note: it may be that we just have too complex a CI pipeline and instead we need to slim things down.

Background

Currently we run a single CI matrix for both "latest" and "develop", this is now causing issues because "latest" supports a different minimum Python to "develop" (Python 3.9). This is leading to CI failures in all the kits.

What can we do?

Option 1

We decouple things and have a set of jobs specific for latest and develop

Option 2

We do something more complicated where we extract the python range for both latest and develop and move things around with if statements (error prone in my opinion).

Option 3

Something else.

orbeckst commented 7 months ago

"Decouple" sounds sane. Is it much more complicated?

IAlibay commented 7 months ago

I had a quick look, this seems to be affecting 9/16 registered mdakits.

I would suggest putting this at the top of the priority queue.

IAlibay commented 7 months ago

"Decouple" sounds sane. Is it much more complicated?

In its simplest form, just make two copies of the action job and make one develop and the other latest.

You can go fancier here by using re-usable workflows, but you have to be careful because too fancy will also be difficult for downstream folks to use it.

orbeckst commented 7 months ago

Once we have a template, we need to write up a mini update guide so that kits not maintained by us can also perform the update.

Perhaps a simple patch may actually work for the majority of cases?

IAlibay commented 7 months ago

I'm possibly ignoring something really simple, but this fix is going to require a reasonable amount of changes.

Possibly the answer here might be to tell folks to use the cookiecutter to create a new template repository and copy the workflow across to their existing repo?

IAlibay commented 7 months ago

We could also just drop develop testing altogether - it's not like even MDA tests against develop numpy properly (I've got an incomplete PR to fix that)

IAlibay commented 7 months ago

Although no develop testing would mean that folks wouldn't be able to do anything if the registry told them they were failing against develop :/

IAlibay commented 7 months ago

One option here is to remove develop testing from the main CI workflow and then add a completely different workflow yaml for develop testing that folks can stick on a cron job if they want? - that way the minimum fix is just "remove develop testing by removing this one keyword", and the more involved fix is "add this new workflow file".

IAlibay commented 7 months ago

I've been thinking about this today as I have another similar problem elsewhere - I'm starting to come around to a different solution for a "minimum fix".

Instead of creating two workflows, setting these matrix exclusions may be easier for folks to update to?

That's a 6-10 lines modification that could be easier to explain?

Long term it's maybe not a great solution though :/

lilyminium commented 7 months ago

That'd certainly be easier, but it's not a given that latest/develop would have Python version mismatches, right -- the release schedule doesn't overlap perfectly with NEP29/SPEC0? I'm still leaning towards multiple workflow files being the easier solution.

orbeckst commented 6 months ago

@lilyminium do we have a recommendation how to fix MDAKits? I am currently reviewing a PR in waterdynamics so https://github.com/MDAnalysis/waterdynamics/issues/34 is now quite pertinent.

lilyminium commented 6 months ago

@orbeckst in the last EOSS4 meeting we settled on the approach in #111 that reverts the test matrix to manual Python and excludes incompatible combinations. The main outstanding issue in #111 following Irfan's review is I need to figure out how to handle codecov API tokens, since codecov uploads can also cause CI to fail for forks. (The temporary solution I first put into #111 was to set fail_ci_if_error: false).

orbeckst commented 6 months ago

Thanks!

(And thank you @IAlibay for doing the fixes.)