Skip to content

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Add trim tests #665

wants to merge 19 commits into from

Conversation

RomeoV
Copy link
Contributor

@RomeoV RomeoV commented Aug 2, 2025

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:

  1. We need to dev seven packages to appease JET and juliac. Each package's reason is documented in the new 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.
  2. The testing code includes clean_optimization.jl and trimmable_optimization.jl. One day hopefully both will be trimmable. At the moment both versions pass JET (impressive!) but only the heavily preallocating version passes juliac.
  3. In order to run juliac we have to put the code to trim into a package. It seems a module will not do. This is documented in the src/TrimTest.jl file.
  4. This is all Julia 1.12.0-rc1 only. But the runtests.jl should take care of that.

@RomeoV
Copy link
Contributor Author

RomeoV commented Aug 2, 2025

Ready for review.

@RomeoV
Copy link
Contributor Author

RomeoV commented Aug 2, 2025

I think the two main tests to look here are:

I don't think anything else should have been impacted...

@RomeoV RomeoV force-pushed the rv/add-trim-tests branch from b2b0d05 to 3c33263 Compare August 2, 2025 23:25
@RomeoV
Copy link
Contributor Author

RomeoV commented Aug 2, 2025

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).
I just pushed one more commit fixing the formatting, so the whole CI has to run again, but otherwise I'd say it's ready to merge.

@ChrisRackauckas
Copy link
Member

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

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

Copy link
Member

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?

@ChrisRackauckas
Copy link
Member

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.
Copy link
Member

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?

Comment on lines +29 to +30
# 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"}
Copy link
Member

Choose a reason for hiding this comment

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

@AayushSabharwal was this completed?

Copy link
Member

@AayushSabharwal AayushSabharwal Aug 7, 2025

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.

@RomeoV
Copy link
Contributor Author

RomeoV commented Aug 7, 2025

I'm trying to wrap the const cache = init(...) calls into a OncePerProcess as suggested by Jeff in JuliaLang/julia#59215 (comment). However, I am getting this error during trimming

MethodError(f=Base.Enums.namemap, args=(SciMLBase.ReturnCode.T,), world=0x0000000000009661)

@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?

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.

4 participants