Safely update retcodes to enums

[ChrisRackauckas]

I think I found a way that is relatively safe. In all of our code throughout SciML, I noticed that the only thing that was truly done was sol.retcode == :Success, etc. in which case a relatively small amount of functions put over a retcode enum could be non-breaking to all functional uses of the previous retcode. If this is the case, we may finally have a good way to upgrade the one piece I find most hideous in the interface.

Fixes SciML/DifferentialEquations.jl#867

[codecov]

Codecov Report

Merging #274 (2acdba9) into master (329d380) will increase coverage by 0.60%.
The diff coverage is 82.85%.

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
+ Coverage   54.75%   55.35%   +0.60%     
==========================================
  Files          41       42       +1     
  Lines        3008     3035      +27     
==========================================
+ Hits         1647     1680      +33     
+ Misses       1361     1355       -6     

Impacted Files	Coverage Δ
src/SciMLBase.jl	81.81% <ø> (ø)
src/solutions/basic_solutions.jl	100.00% <ø> (ø)
src/solutions/dae_solutions.jl	18.18% <ø> (ø)
src/solutions/nonlinear_solutions.jl	55.55% <ø> (ø)
src/solutions/ode_solutions.jl	96.96% <ø> (ø)
src/solutions/optimization_solutions.jl	47.36% <ø> (ø)
src/solutions/pde_solutions.jl	0.00% <ø> (ø)
src/solutions/rode_solutions.jl	70.96% <ø> (ø)
src/retcodes.jl	80.76% <80.76%> (ø)
src/integrator_interface.jl	46.48% <88.88%> (ø)
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[fredrikekre]

This method is already supported for enums, no?

[ChrisRackauckas]

It is. This and the hacky convert are what I were playing around with for backwards compatibility since NonlinearSolve defined some of its own enums, but I don't think this is the right solution. Instead, expanding the meaning of success and telling people to use successful_retcode(sol.retcode) for things like Success or Terminated (which is, solve successfully reached steady state and terminated before final time point), EXACT_SOLUTION_LEFT (bisection grabbed the left), FLOATING_POINT_LIMIT (bisection successfully went to the floating point limit). Trying to slam everything to Success loses information, "is any form of success" seems like a better query for things like optimization loops.

However, I'll loop back to that after I figure out what's going on with downstream tests. DelayDiffEq.jl is being puzzling.

[ChrisRackauckas]

As expected, the convert overload does lead to invalidations. But, it's not on the top of the list and not really the worst, so as a deprecation path this is fine. I think this means we should try to make sure the deprecation is fully completed within 6 months though.

See was a bigger lift than I thought it would be. Dear Jesus, this simple PR is done.

julia> trees[end-1]
inserting convert(::Type{Symbol}, retcode::SciMLBase.ReturnCode.T) in SciMLBase at C:\Users\accou\.julia\dev\SciMLBase\src\retcodes.jl:4 invalidated:
   mt_backedges:  1: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for Pair{Symbol, Any}(::Any, ::Any) (0 children)
                  2: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for REPL.LineEdit.PromptState(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) (0 children)
                  3: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for REPL.LineEdit.MIState(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) (0 children)
                  4: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for push!(::Vector{Symbol}, ::Any) (0 children)
                  5: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for VSCodeServer.JuliaInterpreter.var"#FrameCode#1"(::Bool, ::Bool, ::Type{VSCodeServer.JuliaInterpreter.FrameCode}, ::Method, ::Core.CodeInfo) (0 children)
                  6: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for JuliaInterpreter.var"#FrameCode#1"(::Bool, ::Bool, ::Type{JuliaInterpreter.FrameCode}, ::Method, ::Core.CodeInfo) (0 children)
                  7: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for LoweredCodeUtils.set_to_running_name!(::Any, ::Dict{Symbol, Symbol}, ::JuliaInterpreter.Frame, ::Dict{Symbol, LoweredCodeUtils.MethodInfo}, ::LoweredCodeUtils.SelfCall, ::Dict{Symbol, Union{Nothing, Bool, Symbol}}, ::Symbol, ::Nothing) (0 children)
                  8: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for LoweredCodeUtils.signature(::Any, ::JuliaInterpreter.Frame, ::Any, ::Int64) (0 children)
                  9: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Artifacts.var"#load_overrides#1"(::Bool, ::typeof(Artifacts.load_overrides)) (0 children)
                 10: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Artifacts.var"#load_overrides#1"(::Bool, ::typeof(Artifacts.load_overrides)) (0 children)
                 11: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for LoopVectorization.add_anon_func!(::LoopVectorization.LoopSet, ::Symbol, ::Expr, ::Expr, ::Int64, ::Nothing, ::Int64) (0 children)
                 12: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Artifacts.var"#load_overrides#1"(::Bool, ::typeof(Artifacts.load_overrides)) (0 children)
                 13: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Artifacts.var"#load_overrides#1"(::Bool, ::typeof(Artifacts.load_overrides)) (0 children)
                 14: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Artifacts.var"#load_overrides#1"(::Bool, ::typeof(Artifacts.load_overrides)) (0 children)
                 15: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for LoopVectorization.Instruction(::Any, ::Any) (1 children)      
                 16: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for setindex!(::Dict{Symbol, Function}, ::Any, ::Any) (2 children)
                 17: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for LoopVectorization.indices_loop!(::LoopVectorization.LoopSet, ::Expr, ::Symbol) (3 children)
                 18: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for LoopVectorization.Instruction(::Symbol, ::Any) (3 children)   
                 19: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for setindex!(::IdDict{Module, Symbol}, ::Any, ::Any) (5 children)
                 20: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for REPL.REPLCompletions.FieldCompletion(::DataType, ::Any) (6 children)
                 21: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for SnoopCompile.var"#_flamegraph_frame#113"(::Bool, ::typeof(SnoopCompile._flamegraph_frame), ::IOBuffer, ::SnoopCompileCore.InferenceTimingNode, ::Float64, ::Bool, ::Set{Module}, ::Nothing) (6 children)
                 22: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for setindex!(::Dict{Symbol, REPL.LineEdit.Prompt}, ::Any, ::Any) (7 children)
                 23: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for convert(::Type{Pair{Symbol, Any}}, ::Pair) (25 children)
                 24: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for LoopVectorization.dottosym(::Any) (28 children)
                 25: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for setproperty!(::Base.RefValue{Symbol}, ::Symbol, ::Any) (97 children)
                 26: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Base.ImmutableDict{Symbol, Any}(::Base.ImmutableDict{Symbol, Any}, ::Any, ::Any) (132 children)
                 27: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered
 MethodInstance for setindex!(::Dict{Symbol, Any}, ::Any, ::Any) (147 children)   
   3 mt_cache

[ranocha]

What's the plan to deprecate the conversion to Symbols and go ahead with the plain enum interface? With Julia v1.8.2 and the current version of Trixi.jl, this is one of the main reasons for invalidations:

julia> using SnoopCompileCore; invalidations = @snoopr(using Trixi); using SnoopCompile

julia> length(uinvalidated(invalidations))
3274

julia> trees = invalidation_trees(invalidations)
...
 inserting convert(::Type{Symbol}, retcode::SciMLBase.ReturnCode.T) in SciMLBase at ~/.julia/packages/SciMLBase/cJ8FF/src/retcodes.jl:323 invalidated:
   mt_backedges:  1: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for push!(::Vector{Symbol}, ::Any) (0 children)
                  2: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for SnoopCompileCore.__init__() (0 children)
                  3: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for setindex!(::Dict{Symbol, Function}, ::Any, ::Any) (2 children)
                  4: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for REPL.REPLCompletions.FieldCompletion(::DataType, ::Any) (4 children)
                  5: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for setproperty!(::Base.RefValue{Symbol}, ::Symbol, ::Any) (8 children)
                  6: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for setindex!(::Dict{Symbol, REPL.LineEdit.Prompt}, ::Any, ::Any) (17 children)
                  7: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for convert(::Type{Pair{Symbol, Any}}, ::Pair) (25 children)
                  8: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for setindex!(::IdDict{Module, Symbol}, ::Any, ::Any) (26 children)
                  9: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for setindex!(::Dict{Symbol, Any}, ::Any, ::Any) (176 children)
                 10: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Base.ImmutableDict{Symbol, Any}(::Base.ImmutableDict{Symbol, Any}, ::Any, ::Any) (947 children)

[ChrisRackauckas]

Yeah I knew this was going to be an issue. What I planned to do was first get this all pushed through, and now go add a depwarn now that it has stabilized, and then with v1.9 rip the bandaid off. There's a few codes that used === instead of == so we had to root those out. I think this is something worth pushing through a little fast because it's about correctness and compile times, and at least the worst that happens is someone gets an informative error message so the fallback isn't bad.

[ranocha]

Okay, sounds reasonable. Let me know when you need help with this (in some parts of the ecosystem). I will at least handle Trixi.jl etc.

[ChrisRackauckas]

Everything should already be updated by now. But there may be a few places, we'll have to see when the depwarns start throwing.

[ChrisRackauckas]

#305

[gerlero]

No hard feelings (in fact, I'm absurdly grateful for having access to all of these packages!), but this change unexpectedly broke my code (as an indirect user of this package via OrdinaryDiffEq who was doing ===/!== .retcode comparisons as the docs show).

Even though a fix was easy after pinpointing the cause, I want to voice my opinion that major version bumps be considered as much as possible for changes that will break stuff for us downstream.

In any case, I'm happy that the packages are constantly being improved—and thanks again for all the work!

[ChrisRackauckas]

who was doing ===/!== .retcode comparisons as the docs show

Where did the documentation show that? Anywhere that's shown I'll need to update. The documentation was always showing == and != because those are dispatching which gave us a way to safely update. === doesn't dispatch so we couldn't update. This change was setup a few years ago so that this could be done safely. I'm sorry this led to a bit of friction but if things had all followed the documentation it should've all went smooth. Because the documented form was still okay with this change (given the conversions), this was deemed non-breaking since code didn't break.

That said, it is hard to blame you though since we had a few libraries violate this ourselves in the tests. Since the symbols got ossified over 6 years, there were a few cases where people went "off script" and did a few things we did not expect, like defining their own return codes and modifying them into the solver, along with checking against :Succes and only finding out there was a typo when enums made the check change (which was the point of their change BTW!). Because of this, this time around we are much more clear on enum checking and have made the documentation of the interface much more thorough. But at least this should all be done now and we aren't expecting any major changes to the SciMLBase infrastructure in the near future.

[gerlero]

who was doing ===/!== .retcode comparisons as the docs show

Where did the documentation show that? Anywhere that's shown I'll need to update. The documentation was always showing == and != because those are dispatching which gave us a way to safely update. === doesn't dispatch so we couldn't update. This change was setup a few years ago so that this could be done safely. I'm sorry this led to a bit of friction but if things had all followed the documentation it should've all went smooth. Because the documented form was still okay with this change (given the conversions), this was deemed non-breaking since code didn't break.

https://diffeq.sciml.ai/stable/types/ode_types/#Solution-Type is still using the === comparisons for .retcode. It also mentions "the return code section of the DifferentialEquations.jl documentation" (it's not linked though), which I think refers to https://diffeq.sciml.ai/stable/basics/solution/#retcodes, which has no sample code and also states that retcode is a symbol.

[ChrisRackauckas]

Thanks, updating the docs for that in #318