-
Notifications
You must be signed in to change notification settings - Fork 115
Support struct revision
#894
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
Conversation
I think that's a valid way to say it, but just to clarify, nothing about the type object itself changes, just the name by which you have to access it
yes
I don't understand what you're asking
It lists all cross-module edges to get everything, you also need to iterate all methods in the same module as the binding.
In the fullness of time, the correct way to implement Revise is to run the toplevel expressions in a non-standard evaluation mode that tracks dependency edges for any evaluated constants. However, that should maybe wait until after the JuliaLowering changes and the interpreter rewrite. I think your proposed strategy is reasonable in the meantime although not fully complete. |
e1a5084 to
3f878e3
Compare
I was wondering whether some kind of callback in there could be helpful in short-circuiting the process of determining which methoddefs need to be re-evaluated, but if it's not called then that won't help. From what I can tell, that's not going to work anyway because it only traverses the module containing the type definition. But your comment
seems to be what I was looking for. This seems fairly straightforward, thanks for the excellent groundwork. |
No, there's no edges of any kind for signatures. You need to scan the entire system. However, an earlier version of that code did the same whole-system scan, so you can steal some code for whole-system scanning. |
|
I added a cross-module test (the julia> b = convert(Core.Binding, GlobalRef(StructConst, :Point))
Binding StructConst.Point
40743:∞ - constant binding to StructConst.Point
40598:40742 - constant binding to @world(StructConst.Point, 40598:40742)
1:0 - backdated constant binding to @world(StructConst.Point, 40598:40742)
1:0 - backdated constant binding to @world(StructConst.Point, 40598:40742)
julia> b.backedges
ERROR: UndefRefError: access to undefined reference
Stacktrace:
[1] getproperty(x::Core.Binding, f::Symbol)
@ Base ./Base_compiler.jl:55
[2] top-level scope
@ REPL[6]:1so I don't think I understand what
really means. It also doesn't populate if I add struct PointWrapper
p::StructConst.Point
endto that module. I understand that I have to traverse the whole system, I'm just curious about what |
|
There's two kinds of edges that are tracked there. One is explicit import/using. The other is to lowered code of method definitions (but only after the first inference). At no point is an edge ever added for an evaluation of a binding, only for compilation of code that references the binding. |
|
Just to cross-check: yields a backedge for the julia> e = only(b.backedges)
Binding StructConstUser.Point
40598:∞ - explicit `using` from StructConst.Point
1:0 - undefined binding - guard entry |
That's because |
|
This is getting close, but there's a philosophical question to answer: should Revise try to hew to Base behavior as closely as possible, or should it add/modify behaviors for what might be a nicer interactive experience? In this case, the issue is the following: when I redefine I should say that initially I wasn't aiming in this direction, and this current proposal arose from noticing some unexpected subtleties about the order in which Revise's cache files get set up: the order in which you parse the source, lower the code, and rebind the name matters quite a lot, and some of the choices in this version are designed to compensate for the fact that Revise normally doesn't parse a file unless it's been edited. (Now, we'll also have to parse any file with a method whose signature references an updated type.) I think it's possible to hew to the Base behavior, if we decide that's better, but the initial bugs I faced led me to ask what behavior we actually want, and I came to the conclusion that it's likely a better user experience if we invalidate methods that only work on old types. |
I think that's fine. Conceptually Revise does both adding new methods and deleting old ones. Base is always "append only". |
|
The collateral damage to tests is something I'm looking into; if one runs all the tests, there appear to be some failures that predate this effort, including an incomplete updating of the ecosystem to JuliaLang/julia#52415. I'll work on that. But meanwhile, this seems to be working: tim@diva:~/.julia/dev/Revise$ ~/src/juliaw/julia --startup=no --project -e 'using Pkg; Pkg.test(; test_args=["struct/const revision"], coverage=false)'
Testing Revise
Status `/tmp/jl_4EbO8B/Project.toml`
# Pkg output deleted
Testing Running tests...
Test Summary: | Pass Total Time
Revise | 30 30 10.3s
beginning cleanupand may be ready for anyone who wants to review it. |
e8ba7ab to
7b6d113
Compare
|
@Keno @vtjnash @aviatesk @JeffBezanson here's a fun one: julia> using Revise
Precompiling Revise finished.
1 dependency successfully precompiled in 6 seconds. 19 already precompiled.
julia> Revise.track(Core.Compiler)
julia> fieldnames(Core.Compiler.NativeInterpreter)
(:world, :method_table, :inf_cache, :codegen, :inf_params, :opt_params)
#
# edited Compiler/src/types.jl to add a field `dummy` to `NativeInterpreter`
#
julia> 1+1
WARNING: Detected access to binding `Compiler.#NativeInterpreter#476` in a world prior to its definition world.
Julia 1.12 has introduced more strict world age semantics for global bindings.
!!! This code may malfunction under Revise.
!!! This code will error in future versions of Julia.
Hint: Add an appropriate `invokelatest` around the access to this binding.
⋮ # lots more like this
Compiling the compiler. This may take several minutes ...
Base.Compiler ──── 1.72376 seconds
2
julia> fieldnames(Core.Compiler.NativeInterpreter)
(:world, :method_table, :inf_cache, :codegen, :inf_params, :opt_params, :dummy)
julia> function f(::Integer)
Base.Experimental.@force_compile
return 1
end
f (generic function with 1 method)
julia> f(5)
1 |
|
This is interesting. So it looks like this PR tracks edges not only from binding to method signature, but also from binding to method body? I think a similar mechanism will be needed when developing a new language server. If it's already implemented in Revise, I'd like to reuse it. I haven't had a chance to look at this PR closely yet, but I'll read it later. |
|
Yes, when a type gets rebound, Revise will scan the entire system for methods There's a separate bit of novelty for |
|
I'll be curious to see whether it improves the efficiency of hacking on inference. |
I don't know if I like that. We already have |
|
I didn't know about I can back that commit out, if there are other mechanisms to achieve the same aim. Are you saying that |
|
I tried this. On:
What I did was:
with the following result: julia> using Revise, Tensors
# updates Tensors file here
julia> 1
ERROR: UndefVarError: `Broadcasted` not defined in `StaticArrays.StableFlatten`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
[1] top-level scope
@ ~/.julia/packages/StaticArrays/LSPcF/src/broadcast.jl:180
Revise evaluation error at /Users/kristoffercarlsson/.julia/packages/StaticArrays/LSPcF/src/broadcast.jl:180
Stacktrace:
[1] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{…}, mod::Module, ex::Expr; mode::Symbol, disablebp::Bool, always_rethrow::Bool, kwargs::@Kwargs{})
@ Revise ~/JuliaPkgs/Revise.jl/src/lowered.jl:306
[2] eval_with_signatures(mod::Module, ex::Expr; mode::Symbol, kwargs::@Kwargs{})
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:556
[3] eval_with_signatures
@ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:553 [inlined]
[4] instantiate_sigs!(modexsigs::OrderedCollections.OrderedDict{…}; mode::Symbol, kwargs::@Kwargs{})
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:564
[5] instantiate_sigs!
@ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:560 [inlined]
[6] maybe_extract_sigs_for_meths(meths::Set{Method})
@ Revise ~/JuliaPkgs/Revise.jl/src/pkgs.jl:162
[7] (::Revise.var"#107#108"{Bool})()
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:941
[8] lock(f::Revise.var"#107#108"{Bool}, l::ReentrantLock)
@ Base ./lock.jl:335
[9] #revise#104
@ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:844 [inlined]
[10] revise()
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:842
[11] top-level scope
@ REPL:1
caused by: UndefVarError: `Broadcasted` not defined in `StaticArrays.StableFlatten`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
[1] lookup_var
@ ~/.julia/packages/JuliaInterpreter/J7J9G/src/interpret.jl:5 [inlined]
[2] step_expr!(recurse::Any, frame::JuliaInterpreter.Frame, node::Any, istoplevel::Bool)
@ JuliaInterpreter ~/.julia/packages/JuliaInterpreter/J7J9G/src/interpret.jl:44
[3] signature(recurse::Any, frame::JuliaInterpreter.Frame, stmt::Any, pc::Int64)
@ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/BIVzf/src/signatures.jl:50
[4] methoddef!(recurse::Any, signatures::Vector{Any}, frame::JuliaInterpreter.Frame, stmt::Any, pc::Int64; define::Bool)
@ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/BIVzf/src/signatures.jl:595
[5] methoddef!
@ ~/.julia/packages/LoweredCodeUtils/BIVzf/src/signatures.jl:534 [inlined]
[6] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{…}, frame::JuliaInterpreter.Frame, isrequired::Vector{…}; mode::Symbol, skip_include::Bool)
@ Revise ~/JuliaPkgs/Revise.jl/src/lowered.jl:354
[7] methods_by_execution!
@ ~/JuliaPkgs/Revise.jl/src/lowered.jl:317 [inlined]
[8] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{…}, mod::Module, ex::Expr; mode::Symbol, disablebp::Bool, always_rethrow::Bool, kwargs::@Kwargs{})
@ Revise ~/JuliaPkgs/Revise.jl/src/lowered.jl:296
[9] eval_with_signatures(mod::Module, ex::Expr; mode::Symbol, kwargs::@Kwargs{})
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:556
[10] eval_with_signatures
@ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:553 [inlined]
[11] instantiate_sigs!(modexsigs::OrderedCollections.OrderedDict{…}; mode::Symbol, kwargs::@Kwargs{})
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:564
[12] instantiate_sigs!
@ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:560 [inlined]
[13] maybe_extract_sigs_for_meths(meths::Set{Method})
@ Revise ~/JuliaPkgs/Revise.jl/src/pkgs.jl:162
[14] (::Revise.var"#107#108"{Bool})()
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:941
[15] lock(f::Revise.var"#107#108"{Bool}, l::ReentrantLock)
@ Base ./lock.jl:335
[16] #revise#104
@ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:844 [inlined]
[17] revise()
@ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:842
[18] top-level scope
@ REPL:1
Some type information was truncated. Use `show(err)` to see complete types.
julia> 1
Compiling the compiler. This may take several minutes ...
Base.Compiler ──── 3.43502 seconds
1The UndefVarError seems correct: julia> Tensors.StaticArrays.StableFlatten.Broadcasted
ERROR: UndefVarError: `Broadcasted` not defined in `StaticArrays.StableFlatten`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
[1] getproperty(x::Module, f::Symbol)
@ Base ./Base_compiler.jl:48
[2] top-level scope
@ REPL[4]:1so I guess the question is why it is tried to be looked up. And also, why does the Compiler recompile when I press another time 1 with no new modifications? |
|
I updated against the latest master branch. I tested this branch with the simple case, but it seems that for module Example
export hello, Hello
struct Hello
who::String
end
hello(x::Hello) = hello(x.who)
"""
hello(who::String)
Return "Hello, `who`".
"""
hello(who::String) = "Hello, $who"
endWe get the following error: julia> using Revise
julia> using Example
julia> hello(Hello("world"))
"Hello, world"
julia> # Apply the following diff
# ```diff
# diff --git a/src/Example.jl b/src/Example.jl
# index 65c7eae..c631814 100644
# --- a/src/Example.jl
# +++ b/src/Example.jl
# @@ -2,10 +2,10 @@ module Example
# export hello, Hello
#
# struct Hello
# - who::String
# + who2::String
# end
#
# -hello(x::Hello) = hello(x.who)
# +hello(x::Hello) = hello(x.who2 * " (changed)")
#
# """
# hello(who::String)
# ```
julia> hello(Hello("world"))
ERROR: MethodError: no method matching @world(Example.Hello, 38355:38383)(::String)
The type `@world(Example.Hello, 38355:38383)` exists, but no method is defined for this combination of argument types when trying to construct it.
Stacktrace:
[1] top-level scope
@ REPL[5]:1
julia> hello(Example.Hello("world")) # but this works
"Hello, world (changed)" |
db02a78 to
9b73a5d
Compare
60a75c8 to
792e578
Compare
792e578 to
5c0766a
Compare
|
Ah, it's been a long journey. This work is finally wrapping up. I think Revise can now handle many cases for updating Starting today, I'll spend a few days testing it, and the plan is to merge it around the new year. @timholy I've re-evaluated and implemented the invalidations Revise needs to handle to enable struct revision in most of realistic cases. This ended up adding nearly 1000 new lines of changes, and the logic has been significantly rewritten from the initial struct revision implementation, so I'd like to leave a few points to note for future reference:
|
|
@jkrumbiegel As far as I've tested on my end, the issue you found seems to be resolved. It would be helpful if you could try it again on your side too. julia> using Revise
Precompiling Revise finished.
1 dependency successfully precompiled in 6 seconds
julia> using CairoMakie
julia> fieldnames(CairoMakie.Makie.ScrollZoom)
(:speed, :reset_timer, :prev_xticklabelspace, :prev_yticklabelspace, :reset_delay)
julia> lines(cumsum(randn(100)))
julia> fieldnames(CairoMakie.Makie.ScrollZoom) # after edit (reset_timer -> reset_time)
(:speed, :reset_time, :prev_xticklabelspace, :prev_yticklabelspace, :reset_delay)
julia> lines(cumsum(randn(100))) |
|
@aviatesk, thanks for testing although I did make some changes to Makie which resolved the main issue. So this was not necessarily Revise's fault (it did not trip it up before of course). If you want to see whether your changes also made a difference you'd have to compare with a version before MakieOrg/Makie.jl#5402 was merged. The warnings I mention over there are gone in your example so that would be a big improvement anyway. |
|
I ran the same test on Makie and CairoMakie with MakieOrg/Makie.jl#5402 reverted, and it seems to be working without warnings. |
5c0766a to
4ad1e1a
Compare
|
This is working pretty well for me locally so far! Nice work @aviatesk .
I suspect there are a lot of complexities associated with trying to respond to updates to mutable bindings so I wouldn't expect that to work (yet?), but I did almost immediately run into the case that const IdVector = Vector{Int} # edit and `Graph` is not updated
struct Graph
allocated::IdVector
conflicted::IdVector
resolved::IdVector
end |
|
Same limitation seems to apply for type-aliases: IdArray{N} = Array{Int64,N} # change and `Graph` is not updated
struct Graph
allocated::IdArray{1}
conflicted::IdArray{1}
resolved::IdArray{1}
endIs that expected? This one is a little weird, because it is a type you're re-defining at top-level. It's just not a |
Thanks for testing this! And yeah, Revise doesn't handle this case right now. It's what I'd call a 'binding revision' case, rather than a 'struct revision'. But I think we could support it by adding a simple 'ad-hoc hack'. The same goes for the first case you mentioned; we could add that before merging this PR (added a new TODO item for the OP). |
4ad1e1a to
c609a91
Compare
|
Gave it a shot, but it turned out to be tricky to handle even simple cases where const binding or type alias is used in struct fields with just a simple ad-hoc hack. believe we need a more comprehensive approach to support "binding revision", but I don't think it should be implemented in this PR. So I've removed it from this PR's blocking items. |
Add a dependent module test
Add method scan
Implement recursive processing
The `PointWrapper` binding invalidation only happens when we re-evaluate
the constructor with the new binding for `Point`.
Broaden recursive processing
Fix type-change check, invokelatest
`Core._equiv_typedef` only checks the fieldnames, not their types.
Wrapping `eval_with_signatures` in `invokelatest` ensures that lowering will use the new binding resolutions.
Use subtyping for where constraints
delete debugging code
Perform type-comparisons after unwrapping
Unwrap before querying typename
Check equivalence recursively (fix recursive types)
Types like
```
struct Foo
x::Vector{Foo}
end
```
were getting needlessly redefined.
Fix paths/cachefiles for stdlibs, Compiler
With the removal of some stdlibs, the corresponding cachefiles are no
longer in the base source cache.
The independent Compiler module also needs path fixes.
Increase `mtimedelay`
On fast systems the current setting is too aggressive
Disable tests: tracking CoreCompiler, coverage
For reasons not yet understood, tracking Core.Compiler deletes methods
for `NativeCompiler`.
The "misc - coverage" tests also fail in interactive mode
(`throwto_repl` errors, as it should, in interactive mode).
Improve the mtimedelay change
Avoid redefining types unless required
Improve method redefinition
improve struct revision tests
bootstrap the compiler if invalidated
Update to #918
Update to julia#58131
More mtimedelay tweaking
WSL was still flaky
Fix struct/const revise
Minor Recipes fix
Add aviatesk example
use `Core.methodtable` instead of `Core.GlobalMethod`
Don't call `Compiler.bootstrap!`
Fix 1.10
Force some precompilation
Restore & refine compiler revision
Undo bce9202, but track world ages to distinguish method redefinitions
triggered by Revise from those triggered by ordinary code loading.
More logging improvements
Add timing info, and show MethodSummary arrays.
Update to CodeTracking v2
Back out the test-package precompilaton
Was causing failures in extra tests
includet: avoid "prior to def. world" warning
Add test where field type changes
Fix primitive type comparison
add test with additional constructors
Fix custom constructors
Co-authored-by: Tim Holy <tim.holy@gmail.com>
3fbcba8 to
4ee6596
Compare
|
I've been using this for ~6h in a side project of mine since the latest set of commits was pushed yesterday, and I get a lot of value out of it! Thanks for this great work! I didn't do any dedicated "let's try to break this" but rather just used it organically as part of my regular workflow. I've found it to be very robust in that context. |
|
@lassepe, thanks for testing it. Glad it worked for your case. |
|
Alright, let's merge this. |
UPDATED 2025-12-29
Revise should now be able to handle many cases of updating
structdefinitions.It would be great if you could give it another try, and ping me (@aviatesk) if you find any issues.
Status
MethodInfo->ExInfo)ExInfodata structure to haveSigInfo&TypeInfoThe aim here is to provide full support for struct/const revision. This first commit just adds simple tests, but it already reveals something that needs to be addressed: when a
structgets revised, methods defined for thatstructaren't automatically re-evaluated for the new type. Specifically, in the added tests, after revising the definition ofPointwe getand there is no defined method for a currently-valid
StructConst.Point.@Keno, if you have a moment, let me check my understanding of the current situation:
invalidate_code_for_globalref!is not called?)binding.backedgesto list everything that uses this bindingPresumably, waiting to do the last step as the last stage of a revision would be wise, as it is possible that more than one struct will be revised at the same time, and one might as well do each evaluation only once.