-
Notifications
You must be signed in to change notification settings - Fork 20
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
Hypothesis hotfix exclusion #237
Conversation
JamesRobertsonGames
commented
Aug 19, 2024
•
edited
Loading
edited
- Corrected the NumPy2 hotfix: Adjusted the hotfix to allow specific patches to be excluded, providing more control over which patches are applied.
-
numpy2_patch.json
Outdated
@@ -1765,26 +1765,6 @@ | |||
"original": "numpy >=1.15", | |||
"updated": "numpy >=1.15,<2.0a0" | |||
}, | |||
"hypothesis-6.100.1-py310h06a4308_0.tar.bz2": { |
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.
Hypothesis introduced support for numpy 2 in 6.100.2. So I think we still need the upper bound on 6.100.1.
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.
Yeah just saw your message, we're going to have to build a newer version for defaults anyway so will swing back to getting a newer version on defaults
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.
If I understand correctly, we shouldn't need to run the generate patch a lot anymore. Anything using numpy currently that is updated SHOULD put these protections in place. Going forward, we should only need to remove from the .json
file as needed.
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.
Absolutely agree, this is for a very small subsection during this period of transition and for any important packages that may not have active maintainers
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 call. Let's get this in!
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.
These changes look good to me.
@JamesRobertsonGames Why is the diff in the tests so big and full or R stuff? |
Not sure what this means @JeanChristopheMorinPerso ? There isn't a diff in tests or any relation to R stuff? The code changes are to reflect the oversights in the implementation of the protection dict which was untested at the time of deployment |
Oh I see what you mean now. It seems to happen in all the PRs which is strange but uniform behaviour My code doesn't actually currently impact the hotfix code at all and is not run at all, so we can be 100% confident this is normal behavior (to run the N2 hotfix it spits out a JSON file to be ingested to make the changes at bulk, this has not changed at all) |
@JeanChristopheMorinPerso just want to check your thoughts on the logs with this information inline?
|
JC approved verbally in a meeting so 2 approvals in |