Skip to content

Add JET and Aqua Tests to Subpackages #2665

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 31 commits into
base: master
Choose a base branch
from

Conversation

jClugstor
Copy link
Member

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

This adds JET test_package https://aviatesk.github.io/JET.jl/dev/jetanalysis/#JET.test_package in the typo mode to catch typos in every subpackage.

#2663

@jClugstor jClugstor changed the title Add JET type tests to test each subpackage Add JET typo tests to test each subpackage Apr 4, 2025
@ChrisRackauckas
Copy link
Member

Can you also do a full Aqua QA test?

@gdalle
Copy link
Contributor

gdalle commented Apr 7, 2025

Note that you should probably be careful with JET versioning, for instance JET.test_package is not yet fully supported by JET v0.10 (see https://github.com/aviatesk/JET.jl/releases/tag/v0.10.0)

@jClugstor jClugstor changed the title Add JET typo tests to test each subpackage Add JET and Aqua Tests to Subpackages Apr 7, 2025
@jClugstor
Copy link
Member Author

Many of these are legitimate JET/Aqua failures. Adding these tests without taking care of those issues (if we can) will add a lot of noise when looking at test failures. I can disable the failing tests for each package though to get it set up.

@oscardssmith
Copy link
Member

IMO it's probably at least worth a try to fix the issues.

@ChrisRackauckas
Copy link
Member

Do the same as what was done for the other repos. Cut it down to subsets, disable what will take too much time for now and file issues on those, but get what we can https://github.com/SciML/ModelingToolkit.jl/pull/2364/files#diff-77acaaefd44e4d00398e2700bc87be028acc6625d6c01c480214e2b992929c0aR3

@jClugstor
Copy link
Member Author

Jet complains about DAECache not being defined in OrdinaryDiffEqCore, and I can't find any reference to it besides here: https://github.com/SciML/OrdinaryDiffEq.jl/blob/550948230a176ff428875d66356be600e5a59988/lib/OrdinaryDiffEqCore/src/solve.jl#L500C11-L500C33. Where is DAECache defined?

@oscardssmith
Copy link
Member

nowhere! this is the fun part about JET. It catches your totally broken code.

@jClugstor
Copy link
Member Author

I'm guessing there are no tests with recompile_flag == false and !(_alg isa OrdinaryDiffEqAlgorithm). Is that just a really uncommon scenario?

@oscardssmith
Copy link
Member

both halves of that are unlikely yeah.

@jClugstor jClugstor force-pushed the JET_tests branch 2 times, most recently from fde6d8f to 09f3145 Compare April 24, 2025 17:34
@jClugstor
Copy link
Member Author

Most of the failures are from the pre tests since JET 0.9 doesn't work on Julia 1.12. Is there a nice way to specify that for Julia 1.12 we should use Jet 0.10? Or should I just turn off the JET tests based on the VERSION ?

@@ -23,10 +23,14 @@ SymbolicIndexingInterface = "0.3.38"
Test = "1.10.0"
UnPack = "1.0.2"
julia = "1.10"
JET = "0.9.18"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
JET = "0.9.18"
JET = "0.9.18, 0.10"

This should support Julia 1.12

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that but wouldn't that make it always use JET 0.10, unless JET has a compat restriction for Julia?

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that JET 0.10 is marketed as experimental, especially for test_package

Copy link
Member Author

Choose a reason for hiding this comment

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

This just uses it for typos, so hopefully that would be fine. Anyways, it doesn't work because it needs Julia 1.12.0-beta1.11, but pre only uses Julia 1.12.0-beta1. Very cool.

Copy link
Member

Choose a reason for hiding this comment

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

It's `JET's job to provide appropriate compats (and I believe it does so).

Julia 1.12.0-beta1.11, but pre only uses Julia 1.12.0-beta1. Very cool.

never mind. That sounds like a JET bug...

Copy link
Member

Choose a reason for hiding this comment

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

no

Copy link
Member

Choose a reason for hiding this comment

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

my buest guess is there's a missing comma?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does ask you to use a specific branch at https://github.com/aviatesk/JET.jl/blob/master/CHANGELOG.md#0103:
"However, to use JET v0.10.3, you need to use Julia version v"1.12.0-beta1.11" or later. As of April 19, 2025, you need to build Julia from JuliaLang/julia#58009 for using this 1.12 beta version."

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's fine. If you look at https://github.com/aviatesk/JET.jl/blob/v0.10.4/Project.toml it shows that the minimum Julia compat for [email protected] is [email protected]

Copy link
Member Author

@jClugstor jClugstor Apr 25, 2025

Choose a reason for hiding this comment

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

Ok, so probably just a typo there. The real issue is that test_package isn't available in JET 0.10.4, even though the changelog says it should be.

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