-
Notifications
You must be signed in to change notification settings - Fork 96
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
Pin maximum versions of dependencies in pyproject.toml #1029
Conversation
The versions that work for 3.8 don't seem to be compatible with 3.12. Maybe we should drop 3.8 support? EDIT: I switched to using the 3.12 versions and it all works now. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1029 +/- ##
=======================================
Coverage 89.51% 89.51%
=======================================
Files 26 26
Lines 3453 3453
Branches 630 630
=======================================
Hits 3091 3091
Misses 210 210
Partials 152 152 ☔ View full report in Codecov by Sentry. |
pyproject.toml
Outdated
@@ -22,16 +22,16 @@ license = {file = "LICENSE"} | |||
requires-python = ">=3.8" | |||
dependencies = [ | |||
"bokeh", | |||
"mapca>=0.0.3", | |||
"mapca<=0.0.4", |
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.
"mapca<=0.0.4", | |
"mapca<=0.0.5", |
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 made a new release earlier, so we could already ping to that one.
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.
Thanks! I actually think I should leave it as-is, since dependabot should flag it for us almost immediately after this is merged.
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.
Good point!
I'm a bit confused by this. We know that tedana won't work with all older versions of these libraries. For example, tedana is looking for outputs from Similarly, by pinning maximum versions we'll have to proactively update these version maximums rather than paying attention to when new issues arise. Like I said. I am confused. I'm I misunderstanding the reasoning for this? |
We can set minimum versions if you want, but I'm more worried about breaking changes from new releases than people accidentally installing old versions.
I have kept it pinned to 0.0.4 to ensure that dependabot will open a PR soon.
That's what dependabot is for. Once a new release is made, dependabot will open a pull request with the new version pinned, and if CI fails then we'll know something in the new release doesn't work with our code, rather than surprising us with a user's bug report or failing CI on an unrelated PR. |
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.
Besides my one suggested change. I think this is fine.
Having many PRs for version updates will be annoying, but I understand what it's useful & we can deal. I'm slightly concerned what happens if/when tedana
becomes less actively maintained, but we can deal with that in the future.
Setting both minimum and maximum versions seems to work well. I don't think dependabot can flag problematic minimum versions. We could probably create a minimum-version dependency set and run tests on that, like Nilearn does. That's out of scope for this PR though. |
Co-authored-by: Dan Handwerker <[email protected]>
Now that mapca is up to version 0.0.5 any clue how long it should take for the dependency bot to trigger? |
I had hoped that it would happen immediately after merging this PR, but it looks like we have it set up to run weekly. So, since dependabot was first added 3 days ago, I guess it might not run until 4 days from now? |
Closes #1028.
Changes proposed in this pull request: