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

Fix search CLI crash under Python 3.11+ #114

Merged
merged 3 commits into from
Dec 1, 2024

Conversation

kmaziarz
Copy link
Contributor

@kmaziarz kmaziarz commented Nov 29, 2024

Some of the configuration options in the search CLI use mutable dataclasses as defaults; for some reason, this is only detected by Python 3.11 onwards, therefore our CI running under 3.7 - 3.9 did not flag it, but under Python 3.11 the search CLI crashes with an error. What's worse, one of our environment files (environment.yml) did not pin the version of Python, leading to 3.13 being selected, under which the error is present.

This PR makes the following changes:

  • Use the field constructor instead of mutable defaults to restore proper behaviour under all Python versions.
  • Run the CI under all Python versions ranging from 3.8 to 3.13 (the last stable version). In theory, it would likely be enough to test under 3.8 (oldest supported), 3.9 (used by single-step models) and 3.13 (newest supported). On the other hand, the core tests are fast and run in parallel, so testing under all versions shouldn't significantly increase time taken to merge PRs. Having more CI variants increases the chance that we hit some test flakiness and need to rerun a subset of the CIs, but if anything this will motivate us to fix the flaky tests.

@kmaziarz kmaziarz requested review from AustinT and mrwnmsr November 29, 2024 15:31
@kmaziarz
Copy link
Contributor Author

kmaziarz commented Nov 29, 2024

I had some concerns that a more extensive CI means we can hit a limit on GitHub Actions minutes we can use per month, but it seems the limit only applies to private repositories, so we're good on that front 🙂

Copy link
Collaborator

@AustinT AustinT left a comment

Choose a reason for hiding this comment

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

Looks good Kris, thanks!! If the CI usage becomes a problem in the future we can reduce it, but I wouldn't worry about it now.

@kmaziarz kmaziarz merged commit 59a0ab3 into main Dec 1, 2024
9 checks passed
@kmaziarz kmaziarz deleted the kmaziarz/fix-crash-under-py311 branch December 1, 2024 11:46
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.

2 participants