Version 1.5.7 brakes ModelingToolkit

[ufechner7]

Since this morning I could not install ModelingToolkit any longer. I got the error:

The following 1 direct dependency failed to precompile:

ModelingToolkit [961ee093-0014-501f-94e3-6117800e7a78]

Failed to precompile ModelingToolkit [961ee093-0014-501f-94e3-6117800e7a78] to "/tmp/testdepot/compiled/v1.10/ModelingToolkit/jl_TkA4eb".
ERROR: LoadError: MethodError: setproperties_object(::SymbolicUtils.BasicSymbolic{Real}, ::@NamedTuple{metadata::Base.ImmutableDict{DataType, Any}}) is ambiguous.

Candidates:
  setproperties_object(obj::SymbolicUtils.BasicSymbolic{T}, patch) where T
    @ SymbolicUtils /tmp/testdepot/packages/SymbolicUtils/EGhOJ/src/types.jl:92
  setproperties_object(obj, patch::NamedTuple)
    @ ConstructionBase /tmp/testdepot/packages/ConstructionBase/c2lWA/src/ConstructionBase.jl:205

Possible fix, define
  setproperties_object(::SymbolicUtils.BasicSymbolic{T}, ::NamedTuple) where T

Stacktrace:
 [1] setproperties(obj::SymbolicUtils.BasicSymbolic{Real}, patch::@NamedTuple{metadata::Base.ImmutableDict{DataType, Any}})
   @ ConstructionBase /tmp/testdepot/packages/ConstructionBase/c2lWA/src/ConstructionBase.jl:136
 [2] set
   @ /tmp/testdepot/packages/Setfield/PdKfV/src/sugar.jl:197 [inlined]
 [4] setmetadata(s::SymbolicUtils.BasicSymbolic{Real}, ctx::DataType, val::Tuple{Symbol, Symbol})
   @ SymbolicUtils /tmp/testdepot/packages/SymbolicUtils/EGhOJ/src/types.jl:657
 [5] top-level scope
   @ /tmp/testdepot/packages/ModelingToolkit/GJiqn/src/ModelingToolkit.jl:189
 [6] include
   @ ./Base.jl:495 [inlined]
 [7] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:2222
 [8] top-level scope
   @ stdin:3
in expression starting at /tmp/testdepot/packages/ModelingToolkit/GJiqn/src/ModelingToolkit.jl:1

Downgrading ConstructionsBase to version 1.5.6 fixed the issue.

[jw3126]

Thanks! I consider setproperties_object an implementation detail and did not expect it to be overloaded by external packages:

setproperties_object(obj::SymbolicUtils.BasicSymbolic{T}, patch) where T
    @ SymbolicUtils /tmp/testdepot/packages/SymbolicUtils/EGhOJ/src/types.jl:92

If they need to overload, the overload should probalby have signature setproperties_object(obj::SymbolicUtils.BasicSymbolic{T}, patch::NamedTuple)

[ufechner7]

See also: https://discourse.julialang.org/t/modelingtoolkit-fails-to-pre-compile/118361/2 for a discussion in which package this should be fixed.

[devmotion]

There's an open PR (which IMO should also be backported): JuliaSymbolics/SymbolicUtils.jl#634

[ChrisRackauckas]

For backwards compatability can this get a deprecation? We can patch SymbolicUtils a bit but this is pretty disruptive. It's fine for it to only be considered in hindsight but There's many versions of this in earlier Symbolics going back years, and patching all version is much more difficult in comparison to just putting one depwarn.

[jw3126]

Sure feel free anybody to make a PR. I am a little unclear though when exactly you want a warning.

[MilesCranmer]

This broke a bunch of my CI, could this change be reverted?

[aplavin]

Hmm, I thought SymbolicsUtils fixed their bug (judging by comments above)... That package defined a method for an internal ConstructionBase function instead of the corresponding public function.
Are your CI failures also caused by that SymbolicUtils bug, or something different?

[MilesCranmer]

In my env, SymbolicUtils is limited to a version range (by some other package I guess) that wasn’t patched. Unless you were to patch every version of SymbolicUtils, there are some versions with this issue.

I think the right way to solve this is to patch it here. It’s too much of a disruptive change to have it be a minor version release let alone a patch release

[ChrisRackauckas]

We patched all the way down to v0.19 IIRC.

[rafaqz]

I get your sentiments, but adding methods to undocumented internals is really on SymbolicUtils.jl.

Those methods could have had leading underscores to make it really clear they are internals, but they are just to split up fallback setproperties behaviour between tuples and objects as it's different.

They're not documented anywhere as something to rely on.

(But if you PR a satisfactory fix for all it will likely be merged)

[ChrisRackauckas]

We patched down to v0.19 IIRC which was the first version that had done this, so the latest patch of every released major should already have the fix. If that isn't the case, the please be clear on which version was missed.

[rafaqz]

Great, sorry I didn't see your first comment I was replying to @MilesCranmer

[MilesCranmer]

I think it was v1.4 of SymbolicUtils for me, despite v1.7 of SymbolicUtils existing. So I think some package was limiting it from being upgraded to v1.7 for whatever reason (and it only happened on Julia <=1.8). This chain of compat constraints seems to have triggered the bug in my CI.

@ChrisRackauckas maybe you could make a PR to the registry that constrains all versions of SymbolicUtils to be <=1.5.6 of ConstructionBase? That way there is no worry about individual versions.

---

I get your sentiments, but adding methods to undocumented internals is really on SymbolicUtils.jl.

@rafaqz I agree in principle. But I would say if a patch version breaks ~10s of dependencies, then this rule should be broken, and an exception should be made. Especially because the function name doesn't have an underscore and the Julia public keyword does not yet exist. Even if the intentions were to keep it internal, if has been used as external functionality indirectly by 100s of downstream dependencies, then I would say it has basically become public, until the next major version. Breaking such functionality for a patch version increment just causes unnecessary maintenance burden for downstream maintainers like myself. I think it should be reverted unless the 1.5.7 patch was essential.

[ChrisRackauckas]

@ChrisRackauckas maybe you could make a PR to the registry that constrains all versions of SymbolicUtils to be <=1.5.6 of ConstructionBase? That way there is no worry about individual versions.

This is a pretty hard operation. @DilumAluthge do you have a script that could do something like that?

[MilesCranmer]

I think all you would need to do is modify this line: https://github.com/JuliaRegistries/General/blob/f8db78d600845180cb8c2f3f144da08a449f4415/S/SymbolicUtils/Compat.toml#L122

as follows

  ["0.9-3"]
- ConstructionBase = "1.1.0-1"
+ ConstructionBase = "1.1.0-1.5.6"

[aplavin]

a patch version breaks ~10s of dependencies

Is there anything broken aside from older SymbolicUtils versions? JuliaHub search doesn't point to any package using or defining this function: https://juliahub.com/ui/Search?type=symbols&q=setproperties_object.

Note that it's quite unusual to hold packages to a higher versioning standard than Julia itself. Julia minor releases regularly break packages that rely on internals.

[MilesCranmer]

The ~10 was a guesstimate based on the sparse sample seen in this thread (ModelingToolkit, Pluto, SymbolicUtils, SymbolicRegression, DynamicExpressions, ...), I don't know an actual number. On my side I had CI broken for SymbolicRegression.jl and DynamicExpressions.jl which have SymbolicUtils as a weak dependency: https://github.com/MilesCranmer/SymbolicRegression.jl/actions/runs/10567452338/job/29276449034?pr=326#step:6:377. I have not been able to figure out what package is limiting their versions to 1.4.0, which prevents me from getting Chris's patch.

What is the 1.5.7 patch? Is it essential, or can it be rolled back, considering the breakages reported thus far?

[ChrisRackauckas]

It should be bounded now. I did another release that bumps the minor so it's not exactly the same as what @MilesCranmer requested, but very similar. Please up and see if there's any issues left.

[MilesCranmer]

Cool I think this can be closed now. Thanks Chris!

[jw3126]

Feel free to reopen anybody if something is missing here.