-
Notifications
You must be signed in to change notification settings - Fork 35
JET + Mooncake fixes for 1.12 #921
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
base: main
Are you sure you want to change the base?
Conversation
Benchmark Report for Commit 7363432Computer Information
Benchmark Results
|
DynamicPPL.jl documentation for PR #921 is available at: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #921 +/- ##
=======================================
Coverage 85.10% 85.10%
=======================================
Files 36 36
Lines 3954 3954
=======================================
Hits 3365 3365
Misses 589 589 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 15098406626Details
💛 - Coveralls |
c25fd4c
to
d6e9c83
Compare
625eb18
to
7363432
Compare
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.
Definitely happy with the JET thing, probably happy with the Mooncake thing, unsure about the doctests. Won't we have the same problem of error message variation once the prerelease becomes the next release, and our CI will run both that and min
?
@@ -29,6 +28,9 @@ StableRNGs = "860ef19b-820b-49d6-a774-d7a799459cd3" | |||
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" | |||
Zygote = "e88e6eb3-aa80-5325-afca-941959d7151f" | |||
|
|||
[weakdeps] |
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.
What does it mean for something to be a weak dependency of tests?
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.
To be honest, no idea now, I remember needing to add it to satisfy something. It could have been because of the compat entry i.e. if we remove the Mooncake compat entry from test/Project.toml it might be fine. Let me try that
That's true. I contemplated doing it in this PR but didn't — the correct solution IMO is for the doctests to only be built on the current version, and I think the easiest way to do that is to do the doctests as part of the doc building workflow rather than the test workflow. Would you be on board with that? |
Hmm, what about a separate workflow just for doctests? Annoying code duplication? They do feel a lot more like tests than docs to me, build-wise. |
A little bit of workflow yaml code duplication in exchange for not having weird logic inside test/runtests.jl seems like a win to me, let's go with that. |
julia-version = pre
. This removes it from the test suite if the Julia version is a prerelease.Previously, we knew that CI was failing on 1.12 because of Mooncake. However, removing Mooncake also flagged up a couple more errors that I had to resolve to get CI to pass:
JET.jl has a new version, v0.10, that is intended for Julia 1.12. This PR also includes v0.10 in the allowed compat ranges for JET.jl so that the tests can run on
pre
. Previously, the JET tests onpre
would fail as CI would attempt to install an older version of JET that wasn't forward-compatible. Thankfully, this doesn't require runtime checks because Pkg will automatically figure out the appropriate version of JET to install.Finally, this PR disables doctests on 1.12, because error messages vary from version to version and it can be very flaky to test correctly for them.
There is still one remaining test failure on 1.12, which is related to the use of
Threads.threadid()
inThreadSafeVarInfo
. I wrote up more about this problem in #924.Supersedes: #872 #873 #875