-
-
Notifications
You must be signed in to change notification settings - Fork 51
Add trim
tests
#665
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: master
Are you sure you want to change the base?
Add trim
tests
#665
Conversation
We introduce `SafeTestsets` as part of this, inspired by the way the downstream tests are set up in the `SciMLBase` repo.
See TrimTest.jl comment for rationale.
The rational in code comment.
Ready for review. |
I think the two main tests to look here are: I don't think anything else should have been impacted... |
b2b0d05
to
3c33263
Compare
The tests are looking good. Checking the CI results from the previous results, everything that passes on master also passes here. Most importantly, Ubuntu w/ julia-pre, which is the only one that should actually be affected (besides MacOS julia-pre, which crashes for an unrelated reason). |
We shouldn't merge this with stuff still on branches. Need to get the upstreams fixed first. |
return 0 | ||
end | ||
``` | ||
segfaults `juliac`. Looking at the segfault stacktrace it seems the culprit is |
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.
This looks like it's worth a bug report upstream
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.
Claude wasn't able to reproduce it with an MWE, so @RomeoV can you take this?
[skip ci]
This should get updated to CPUSummary v0.2.7 which should fix the Polyester and PolyesterWeave pieces (i.e. remove those bits, and trim should work still just with release) |
StaticArrays = "90137ffa-7385-5640-81b9-e52037218182" | ||
|
||
[sources] | ||
# Remove assert that triggers false positive for JET. Tracked at https://github.com/aviatesk/JET.jl/issues/736. |
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.
@aviatesk can you look into this?
# Fix a type instability. Tracked at https://github.com/SciML/SciMLBase.jl/pull/1074. | ||
SciMLBase = {url = "https://github.com/AayushSabharwal/SciMLBase.jl", rev="as/fix-jet-opt"} |
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.
@AayushSabharwal was this completed?
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.
No, this is running into the JET failures in LinearAlgebra/@assert
in ForwardDiff I mentioned in slack. It fixes the type instability relevant here, we just can't test it outside of 1.11.
I'm trying to wrap the
@ChrisRackauckas I recall that we got this error during the hackathon at JuliaCon. I recall we tried to put something like println(Core.stdout, SciMLBase.ReturnCode.T) into the precompile, but it doesn't seem to help, unfortunately. Do you have any other ideas? |
Add a
trim
directory with basic tests for trimming.I tried to imitate the code/package loading structure of SciMLBase.
There are a few quirks here, most of which hopefully will resolve themselves soon:
test/trim/Project.toml
file. Also, when the upstream branches are merged and deleted, this will fail. But that's good so we can bump the compat accordingly.clean_optimization.jl
andtrimmable_optimization.jl
. One day hopefully both will be trimmable. At the moment both versions pass JET (impressive!) but only the heavily preallocating version passesjuliac
.juliac
we have to put the code to trim into a package. It seems a module will not do. This is documented in thesrc/TrimTest.jl
file.runtests.jl
should take care of that.