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

Don't run CI on mdanalysis latest + latest python #93

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Oct 29, 2023

Fixes #92 (I think)

This might need some discussion.

Currently CI will run all the versions that the develop version of MDAnalysis will support.

This will cause failures when it attempts to run a brand new version of Python that the last release of MDA doesn't support (for example Python 3.12 + MDAnalysis 2.6.1).

There are two options I can see out of this:

  1. We just don't run the latest version of Python against the last release of MDAnalysis - this fix does this.
  2. We split up the test step into two, a latest & development step. On the latest step we take the output of mdanalysis-compatible-python with the latest version of MDA, and the development the same python matrix as what we call currently.
  • Note: This will need a bit of work to grab out the latest version of MDA, it's not the end of the world but I'm unlikely to have much time to do this in the next couple of days given work requirements (indeed this is the last thing I can do today since I have to go back to my day job).

In all cases we're going to have to go to the MDAnalysis cookie-cutter using repos and fix this workflow, because right now all their CI is failing.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG.md updated?
  • AUTHORS.md updated?
  • Issue raised/referenced?

steps:
- uses: actions/setup-python@v4
with:
python-version: "3.11"

- id: get-compatible-python
uses: MDAnalysis/mdanalysis-compatible-python@main
with:
release: "latest"
Copy link
Member Author

Choose a reason for hiding this comment

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

Right this is my short term fix - updated mdanalysis-compatible-python to have a latest option.

This means that this CI matrix has no way of checking for compatibility with the latest versions of Python...

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this and opening this @IAlibay -- for now, LGTM as an interim fix!

However I think I prefer your Option 2 that you outlined there, so that we can keep testing develop with the newest and greatest. Unless I'm missing something, I don't see many downsides aside from additional CI complexity (maybe a slightly larger matrix?) @orbeckst @ianmkenney @fiona-naughton do you have any opinions here?

@lilyminium
Copy link
Member

In all cases we're going to have to go to the MDAnalysis cookie-cutter using repos and fix this workflow, because right now all their CI is failing.

Yeah this is going to be a hassle now and in the future -- when there's time I'd like to look into a bot or similar that can keep watch on changes to the cookiecutter and maybe open PRs with cherry-picked changes when we update cookie-related things.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

I am ok with the band-aid.

(As @lilyminium said, option 2 is better mid-term.)

In all cases we're going to have to go to the MDAnalysis cookie-cutter using repos and fix this workflow, because right now all their CI is failing.

Yikes. Is there a way we can move "this" into an action that we can fix centrally? (Well, discussion for another issue.)

orbeckst pushed a commit to MDAnalysis/mdaencore that referenced this pull request Oct 31, 2023
* Update deployment for OIDC (trusted publisher)
* Exclude latest python from latest mda deploiyment (using hot-fix "latest", see
  MDAnalysis/cookiecutter-mdakit#93 for background)
@IAlibay
Copy link
Member Author

IAlibay commented Oct 31, 2023

In all cases we're going to have to go to the MDAnalysis cookie-cutter using repos and fix this workflow, because right now all their CI is failing.

A long term solution here is great, and I'm happy to have that discussion - indeed there is lots we can do.

However, the intent of that comment was "who takes responsibility for dealing with this today".

I don't want to spend time cycling on this too much, so I've already added the short term fix to PRs to transport-analysis, waterdynamics, pathsimsanalysis and mdaencore.

@ianmkenney @orbeckst @fiona-naughton @lilyminium If there are any other MDAKits that you know need updating, please either chime in here with the list of mdakits or spread the fix around.

@IAlibay
Copy link
Member Author

IAlibay commented Oct 31, 2023

However I think I prefer your Option 2 that you outlined there, so that we can keep testing develop with the newest and greatest. Unless I'm missing something, I don't see many downsides aside from additional CI complexity (maybe a slightly larger matrix?)

This is to be discussed elsewhere, but the current environment.yaml strategy won't work because it will always include MDAnalysis within it, so it won't work unless MDA is on conda-forge already. There are options I can think of but they aren't super user friendly / easy to fix. Very likely I'm missing something obvious though.

Honestly my long term opinion here is that, if we want to build our user community, we should, like numpy and scipy, take on the responsibility of having a Python 3.12 release as soon as possible. However that's a core library time & efforts allocation issue.

@orbeckst
Copy link
Member

Sounds like a good insight, born from experience.

Do we have a document with our maintenance aspirations? At least internally we should have an idea what our goals are.

@lilyminium
Copy link
Member

I don't think we've written down maintenance aspirations in that way before, although of course @IAlibay would know better here. There is a related maintenance SDG application but it focuses more on dev docs and CI/CD utilities.

Not to cut the discussion short, but I might merge this PR given the approvals just we can get CI back for now!

@lilyminium lilyminium merged commit 3824809 into main Nov 1, 2023
12 checks passed
@lilyminium lilyminium deleted the latest-python-fix branch November 1, 2023 05:52
@lilyminium
Copy link
Member

Thank you again @IAlibay for the fix!

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.

cookiecutter Cron CI failed
3 participants