-
Notifications
You must be signed in to change notification settings - Fork 34
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
Bump minor version to 0.15 #353
Conversation
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.
It looks good to me.
Perhaps address #347 before releasing so future PRs don't get broken CI status. |
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.
Compat was already bumped in 0.14.0 so bumping patch on this is fine
@yebai The new rule fixes the Interface tests, so the only failing ones are AD/Enzyme. There isn't any requirement that any CI test passes for PRs (https://github.com/TuringLang/Bijectors.jl/settings/branches), so we can just ignore the AD/Enzyme ones when making PRs? We could hide the red cross (e.g. by not running the tests, or by using |
Given the fragility of Enzyme (e.g. compatibility with new Julia releases or other libraries), we would like to test Enzyme in a new (isolated) test environment, which avoids loading Enzyme in the For example, we have been using isolated environments for integration testing in |
That's a fair way of guarding against errors-on-import spoiling the non-Enzyme jobs, but it doesn't help us with errors-on-use. As long as Enzyme is tested in some CI job, there will be a red cross, so doing this won't affect the CI status of new PRs unless we either disable testing or explicitly hide the red cross with |
That's true, but it's still helpful. It prevents non-Enzyme CIs from frequently breaking due to loading Enzyme.
I think it is okay to enable |
This should give us back the green tick. It doesn't fix the possibility of error-on-import, but that's not currently a problem (as the extension stuff was fixed) so that can be for another time imo |
1.4.2 seems to still say v1.6: https://github.com/TuringLang/Bijectors.jl/blob/v0.14.2/Project.toml#L73 Thanks for the help here @penelopeysm! @yebai, are you happy to leave guarding against the error-on-import stuff for later? |
Yes, please add a clarification on this remaining issue in #347 |
Wut, I guess this got lost somewhere in the bunch of PRs. Anyway we decided that a minor bump is more appropriate for Julia compat bumps: So maybe this should be 0.15.0. I don't know what happened to that PR and why it got lost. |
Now bumping to 0.15. |
Raises minimum Julia version to v1.10 and bumps EnzymeCore compat to latest, with the manual
find_alpha
rule.Could also bump the minor, rather than patch, version, because of the changing Julia compat. This shouldn't cause breakage for anyone who manages to install it, so in that sense it's not breaking, but bumping Julia versions could still be considered a "major" thing. Do we have a convention for this in Julia-land or Turing-land?EDIT: See later comments, decided to go with a minor