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

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

Open
IAlibay opened this issue Mar 17, 2024 · 13 comments
Assignees

Comments

@IAlibay
Copy link
Member

IAlibay commented Mar 17, 2024

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
Copy link
Member

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

@IAlibay
Copy link
Member Author

IAlibay commented Apr 1, 2024

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
Copy link
Member Author

IAlibay commented Apr 1, 2024

"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
Copy link
Member

orbeckst commented Apr 1, 2024

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
Copy link
Member Author

IAlibay commented Apr 1, 2024

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
Copy link
Member Author

IAlibay commented Apr 1, 2024

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
Copy link
Member Author

IAlibay commented Apr 1, 2024

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
Copy link
Member Author

IAlibay commented Apr 1, 2024

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
Copy link
Member Author

IAlibay commented Apr 6, 2024

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?

  • exclude develop with oldest python in the matrix
  • exclude stable with newest python in the matrix

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

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

@lilyminium
Copy link
Member

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
Copy link
Member

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

@lilyminium
Copy link
Member

@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
Copy link
Member

Thanks!

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

orbeckst added a commit to Becksteinlab/propkatraj that referenced this issue May 2, 2024
- fix #69
- only test Python 3.10 - 3.12
- manually specify versions of MDAnalysis in the test matrix that do not
  support some of the python versions (see MDAnalysis/cookiecutter-mdakit#110
  for background)
orbeckst added a commit to Becksteinlab/propkatraj that referenced this issue May 2, 2024
- fix #69
- only test Python 3.10 - 3.12
- manually specify versions of MDAnalysis in the test matrix that do not
  support some of the python versions (see MDAnalysis/cookiecutter-mdakit#110
  for background)
IAlibay pushed a commit to Becksteinlab/propkatraj that referenced this issue May 2, 2024
* update CI matrix

- fix #69
- only test Python 3.10 - 3.12
- manually specify versions of MDAnalysis in the test matrix that do not
  support some of the python versions (see MDAnalysis/cookiecutter-mdakit#110
  for background)

* bumped Python and MDAnalysis versions in pyproject.toml

- Python 3.10+
- MDAnalysis 2.1.0+
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

No branches or pull requests

3 participants