Replace Expronicon with Moshi

[visr]

This is used for the exported Algebraic Data Type (ADT) TimeDomain.

Closes #741 which wasn't merged due to invalidations caused by newer Expronicon releases. Moshi is the replacement by the same author that fixes this issue. It is a different design so some care is taken to minimize disruption.

One benefit of this change is that it removes these dependencies from SciMLBase: Pkg, Expronicon, MLStyle, ArgTools, Downloads, FileWatching, InteractiveUtils, LibCURL, REPL, Tar, LibCURL_jll, MozillaCACerts_jll, Zlib_jll, nghttp2_jll, p7zip_jll

Moshi only depends on ExproniconLite and Jieko.

One of the differences between Expronicon and Moshi is that the former ADT macro creates a type, whereas the latter creates a module with the Type in there. Since the ADT was manually placed in the Clocks module, I could just rename the ADT to the module Clocks, and the old ADT name is kept as const TimeDomain = Clocks.Type. With that TimeDomain can still be used for dispatch or container typing like is done in ModelingToolkit.

The DiscriminatorType code was only added for Clock serialization in #753, but that now works without extra code.

The biggest change that I couldn't work around is that Continuous and SolverStepClock, both singleton variants, should now be explicitly constructed with Continuous instead of Continuous(). My idea was to land and release #921 first, so that ModelingToolkit can update to this new syntax, and then merge this next. Though if you prefer to just update both SciMLBase and ModelingToolkit together then #921 wouldn't be needed.

[ChrisRackauckas]

My idea was to land and release #921 first, so that ModelingToolkit can update to this new syntax, and then merge this next. Though if you prefer to just update both SciMLBase and ModelingToolkit together then #921 wouldn't be needed.

It's not just MTK but also SymbolicUtils.jl that needs it, and it's been a high priority thing to get that update in some time.

But yes let's do this in steps.

[visr]

No longer a draft because SciML/ModelingToolkit.jl#3353 is merged and released.

[AayushSabharwal]

This is nice, but we can't merge this since Moshi ADTs can't be matched via MLStyle. If we do, downstream matching will suddenly fail

[visr]

@AayushSabharwal see also my reply here: SciML/ModelingToolkit.jl#3354. Are you sure these ADTs are matched anywhere with MLStyle? If so we could add the extensible pattern matching here. Though SciMLBase currently has no MLStyle dep, strong or weak.

[AayushSabharwal]

Not all packages are public :D

[AayushSabharwal]

If we can add the pattern matching API as a weakdep this would be good, although at that point might as well PR to Moshi

[visr]

We could ask at the Moshi repo about a weak dep, though since I think the package is supposed to replace MLStyle perhaps they wouldn't want to take on the maintenance.

[visr]

By the way are you interested in moving from MLStyle to Moshi besides Expronicon to Moshi?

[visr]

I made Roger-luo/Moshi.jl#41. I put this PR in draft to avoid breaking code.

[AayushSabharwal]

Is there an easy-ish way to just special-case this ADT to work with MLStyle in an extension?

[AayushSabharwal]

Also note that Expronicon.ADT.@match is the same @match macro from MLStyle

[visr]

Is there an easy-ish way to just special-case this ADT to work with MLStyle in an extension?

Are you looking for any way to match this ADT with MLStyle or such that your old MLStyle match syntax doesn't have to change at all? I suspect the latter will be difficult. I tried before filing Roger-luo/Moshi.jl#41 but didn't manage, but as Roger mentions it may work for a subset.

If you switch to MLStyle active patterns the code below works for this ADT, though at that point I guess you may as well directly update to Moshi match?

using Moshi.Data: @data, variant_type
using MLStyle: @match
using MLStyle.Active: @active

@data Clocks begin
    Continuous
    struct PeriodicClock
        dt::Union{Nothing, Float64, Rational{Int}}
        phase::Float64 = 0.0
    end
    SolverStepClock
end

const TimeDomain = Clocks.Type
using .Clocks: Continuous, PeriodicClock, SolverStepClock

@active IsContinuous(c) begin
    variant_type(c) == Continuous
end

@active IsSolverStepClock(c) begin
    variant_type(c) == SolverStepClock
end

@active PeriodicClockPattern(c) begin
    if variant_type(c) == PeriodicClock
        (c.dt, c.phase)
    else
        nothing
    end
end

function active_match(c)
    @match c begin
        IsContinuous() => "continuous"
        IsSolverStepClock() => "solver_step_clock"
        PeriodicClockPattern(dt, phase) => (dt, phase)
        _ => "other"
    end
end

active_match(Continuous())  # => "continuous"
active_match(SolverStepClock())  # => "solver_step_clock"
active_match(PeriodicClock(1//2, 3.14))  # => (1//2, 3.14)

[AayushSabharwal]

Can @active matches have the same name as the variants? Because if so we can just define those in an extension to SciMLBase and export them

[visr]

This works for the singletons Continuous and SolverStepClock, and supports matching and getting the args out of PeriodicClock. Both args need to be defined, in the right order, kwarg matching doesn't work. I tried to fix that based on expanding @as_record, and fixing based on differences between structs and variants, but it was pretty painful and I couldn't get rid of a v isa PeriodicClock which is false for variants but true for structs. I don't know if you need those features though.

https://gist.github.com/visr/48552b34326d7cad3fb8e558db9abf55

[AayushSabharwal]

We don't need the keyword argument syntax. As long as this works:

sampletime(c) = @match c begin
    PeriodicClock(dt, _...) => dt
    _ => nothing
end

We're good.

[visr]

That now works in the revised Gist. If you're happy with that I can look to make it into a weakdep. It feels quite fragile, I'd really recommend to move to Moshi match as soon as possible. But if this helps to make this PR non-breaking, let's add it, and consider removing it again in the next breaking release of SciMLBase.

[AayushSabharwal]

Yep we just need backward compatibility, so the weakdep approach works.

[visr]

I made the package extension. While adding tests I noticed an issue with the wildcard matching that I cannot figure out. This one works:

sampletime(c) = @match c begin
    PeriodicClock(dt, _...) => dt
    _ => nothing
end

But if you replace nothing with anything else, you will still get nothing. This comes from periodic_clock_pattern, but I don't have access to the match target values there.

[AayushSabharwal]

https://github.com/SciML/SciMLBase.jl/actions/runs/13219508877/job/36902784715?pr=922 is a real failure

[visr]

Hmm I see that test is running with an old MTK that doesn't have SciML/ModelingToolkit.jl#3353.

⌃ [961ee093] ModelingToolkit v9.59.0

[AayushSabharwal]

Try rebasing this branch? SciMLBase has been tagged since this PR was opened and MTK has compat bounds

[visr]

Hmm I rebased yesterday, just did again.

[AayushSabharwal]

I didn't notice that, hmm. Let's see what happens this time

[visr]

Same thing. What I don't understand is that there is a Pkg.update() in the downstream CI, and the MTK version is outdated but not blocked from updating.

SciMLBase.jl/.github/workflows/Downstream.yml

Line 73 in a6b105f

Pkg.update()

[AayushSabharwal]

It might be a more complicated dependency issue. A green up arrow doesn't necessarily mean the package can't be updated, just that nothing explicitly has an upper bound for it

[AayushSabharwal]

Although this does mean that the change isn't backward compatible and will break older versions of MTK

[AayushSabharwal]

What if we rename Continuous to ContinuousClock in the ADT, add const Continuous = ContinuousClock() and define (clock::TimeDomain)() = clock?

[visr]

I found the reason for the old MTK release, by checking out SciMLSensitivity, and activating its test environment:

ulia> using TestEnv; TestEnv.activate()
  0 dependencies successfully precompiled in 1 seconds. 274 already precompiled.
┌ Warning: Could not use exact versions of packages in manifest, re-resolving
└ @ TestEnv a:\.julia\packages\TestEnv\tgnBf\src\julia-1.11\activate_set.jl:63
"C:\\Users\\VISSER~1\\AppData\\Local\\Temp\\jl_DgytJr\\Project.toml"

(jl_DgytJr) pkg> st --outdated
Status `C:\Users\visser_mn\AppData\Local\Temp\jl_DgytJr\Project.toml`
⌅ [7ed4a6bd] LinearSolve v2.39.0 (<v3.0.0): AlgebraicMultigrid, NonlinearSolve, OrdinaryDiffEqDefault, OrdinaryDiffEqDifferentiation, OrdinaryDiffEqExtrapolation, OrdinaryDiffEqFIRK, OrdinaryDiffEqNonlinearSolve, OrdinaryDiffEqRosenbrock, SciMLSensitivity
⌃ [961ee093] ModelingToolkit v9.59.0 (<v9.62.0)
⌅ [8913a72c] NonlinearSolve v3.15.1 (<v4.3.0) [compat]
⌅ [e88e6eb3] Zygote v0.6.75 (<v0.7.4): SciMLSensitivity

Making a release with SciML/SciMLSensitivity.jl#1164 should fix it:.

[AayushSabharwal]

Regardless, we need the backward compatibility fix

[visr]

I went over the failing tests and could only find one related failure, that the number of invalidations went up.

inserting variants(::Type{SciMLBase.Clocks.var"typeof(Clocks)"}) @ SciMLBase.Clocks a:\.julia\packages\Moshi\SEGHC\src\data\emit\reflect.jl:9 invalidated:
   backedges: 1: superseding variants(x::Type) @ Moshi.Data a:\.julia\packages\Moshi\SEGHC\src\data\runtime.jl:88 with MethodInstance for Moshi.Data.variants(::DataType) (1 children)
              2: superseding variants(x::Type) @ Moshi.Data a:\.julia\packages\Moshi\SEGHC\src\data\runtime.jl:88 with MethodInstance for Moshi.Data.variants(::Type) (2 children)
              3: superseding variants(x) @ Moshi.Data a:\.julia\packages\Moshi\SEGHC\src\data\runtime.jl:97 with MethodInstance for Moshi.Data.variants(::Any) (4 children)
   1 mt_cache

This is only invalidating some fallback error methods in Moshi itself, so probably by design:

function variants(x::Type)::Tuple
    throw(IllegalDispatch())
end

[AayushSabharwal]

Looks great!

[pitx-perf]

functions like isclock, issolverstepclock and iscontinuous are not robust to non-TimeDomain arguments anymore.
Particularly, isclock(nothing) throws an error while it was returning false with Expronicon ADT.

[AayushSabharwal]

That's an interesting case. It looks like we don't test it but we should, since the function isn't type-annotated to restrict it to TimeDomain arguments. Could you open an issue so it can be tracked? And in general commenting on closed PRs makes it likely that things end up being forgotten. Please open issues mentioning the problematic PR instead.