-
Notifications
You must be signed in to change notification settings - Fork 110
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
redefining struct #18
Comments
The problem is that then you have two notions of ERROR: MethodError: no method matching foo(::MyModule.MyType)
Closest candidates are:
foo(::MyModule.MyType) at REPL[2]:1 (The first is really |
Here's an actual example: julia> module MyModule
struct MyType end
foo(x::MyType) = 1
end
MyModule
julia> x = MyModule.MyType()
MyModule.MyType()
julia> MyModule.foo(x)
1
julia> module MyModule
struct MyType end
foo(x::MyType) = 1
end
WARNING: replacing module MyModule
MyModule
julia> MyModule.foo(x)
ERROR: MethodError: no method matching foo(::MyModule.MyType)
Closest candidates are:
foo(::MyModule.MyType) at REPL[4]:3 |
I am aware of the issue, but still consider reloading the whole module the best solution. It is recommended in the manual, and is still preferable to restarting Julia, which is the only other workaround I am aware of, and is usually much more time-consuming (especially if one uses I understand your reluctance to deal with this in |
I think I wrote that documentation, but turnabout is fair play 😄. As you probably know, it gets complicated really fast. If you reload A, but have module B that depends on A, you also need to reload B. And (as shown in that manpage) all your variables need to be redefined. Perhaps we could store the entire REPL history for this session and re-execute it? What about statements that, e.g., chop the last 10 bytes off a file, did you really mean to do that twice? The problems seem quite daunting to me. The "reload all dependent modules" problem may be fairly easily solvable in cases of precompilation because the cache file holds a reliable record of dependencies. (For non-precompiled modules we'd have to rely on a parsing strategy, which might work most of the time but would be fundamentally less reliable.) The really scary problem is what to do with old variables. I'm not against the idea of trying to do something, but I don't have any good ideas for how to solve it. |
OK, I confess you got me thinking: JuliaLang/julia#22721 |
I was wondering: instead of renaming the old type, would it be possible to rename the new one instead? It won't get every case, but perhaps just manually calling |
That might be work. We'd still have to use |
It could, but you might also just be able to just look through your parse tree for that symbol. The parse tree will perhaps also be more accurate at finding the actual reference to the type, which doesn't necessarily need to be the same function as the inference tree leading up to an allocation of that type. |
Yuck. julia> struct Foo end
julia> function fakefoo()
Foo = sqrt
return Foo(7)
end
fakefoo (generic function with 1 method)
julia> fakefoo()
2.6457513110645907 Another thing I haven't mentioned yet is that I was planning on trying to scan the |
@cstjean just came up with a surprisingly simple but clever idea which could be an effective workaround: https://discourse.julialang.org/t/advice-for-dealing-with-struct-during-development/21732/6 The only obvious downsides being that you'd have to annotate the @timholy, would you consider this (or something similar) for inclusion in |
One thing which that trick doesn't allow is type parameters, or at least no changing type parameters. |
I'm interested in exploring this. No bandwidth now, though, not until #243 merges and the associated debugger stack is out there. (And also not a great time to make lots of changes in the Revise codebase, I recommend waiting a week or so.) |
It could be useful for those who |
Could this be resolved by having a system similar to Figwheel. I was an avid user of ClojureScript and this made the development lifecycle amazing. They handled situations like this with their guide for writing reloadable code. A quick summary is that they had Additionally hooks were provided |
julia> struct MyStruct
x::Int
end
julia> struct MyStruct
x::Int16
end
ERROR: invalid redefinition of constant MyStruct
Stacktrace:
[1] top-level scope at none:0
julia> struct MyStruct
x::Int
end So Julia only complains if you're making changes. Unfortunately if you've made that change in the source code, you'd like it to take effect. That said, in theory there are places where annotating a block as "load once" might be necessary. An example might be code that initializes a C library, and for which you'd get a crash if you tried to initialize it a second time. I've kind of been amazed that it hasn't really come up yet---either it just doesn't come up, or people aren't reporting problems. I suspect that Julia's precompilation imposes a discipline that makes the large majority of Julia packages well-suited for code-redefinition. |
@BeastyBlacksmith mentioned the following on discourse: https://github.com/BeastyBlacksmith/ProtoStructs.jl |
I guess you already considered this, but I'm curious, why you didn't like those somewhat hacky repl tricks: |
I'm trying to get away from "somewhat hacky": the main goal in Revise development over the last year or so has been to make it Just Work for almost everybody. I'm fairly pleased with where it is now, and hopefully the slow rate of bug reports recently means that others are too. (If not, please report bugs!) I put problems into different categories, and "should work but doesn't" (aka, a "real bug") is far more serious than "a feature that might be nice but isn't properly supported by Julia itself." By https://github.com/timholy/Revise.jl/issues just about the only "real bug" that I know about is #249. Hopefully most of the infrastructure needed to fix it is already in place, though it's pretty complicated. Still, it may be getting to the point where it's worth considering tackling this problem again. I had a brief run at JuliaLang/julia#22721 recently, but I'm back to thinking that's hard. Renaming in the other direction might be the the better approach. I'd certainly welcome PRs, though I'm unlikely to accept a solution that only works sometimes. |
In the context of addressing constant redefinition ala JuliaLang/julia#22721 like JuliaLang/julia#265 (ie with world versioning), we were recently talking about this in the Julialab. It turned out we already have all pieces for it by doing via a small transform: by making a new module that has all the same bindings (but the one) as the old module, we can redefine the type in the new module and then it's defined mechanically if we're referring to the old or new module of that name. But otherwise they have the same bindings (and functions and methods), so using the code won't particularly notice the difference for accessing through the old or new! (Sorry I have no sample code to demo, but happy to answer questions) |
Same name or different? It's pretty easy to do "different," but then that leaves you with questions to answer about how you integrate the new name into other code.
Can you elaborate? |
Same name—since it's a toplevel module, it's just a reference in If you call |
Really interesting. If I understand correctly, everything that references that module needs to be recompiled, right? My biggest concern is that could be a lot of recompilation; suppose you change a type in I should say I had a pretty serious stab at this in my Lines 665 to 731 in bd5f1a0
|
@vtjnash, would be interested in restarting this conversation at some point. With Revise 3 out I think most of the remaining brittleness has been eliminated, and it might be time to consider expanding into new territory. |
@timholy , hat would be great. For my current Julia use case, I have built a framework. Whenever I am just building client code (on top of the framework), Revise works flawlessly. The problem is when I am doing work inside the framework. If I need to come close to any struct I am obliged to restart the environment, which is a productivity killer. I love Julia's language design, but unfortunately the current worflow doesn't suit me and my team very well, to the point that we are considering migration :( I think if there was a workaround this, one of the major paint points of developing in Julia would be solved. |
JuliaLang/julia#40399 is the latest. |
This implements world-age partitioning of bindings as proposed in #40399. In effect, much like methods, the global view of bindings now depends on your currently executing world. This means that `const` bindings can now have different values in different worlds. In principle it also means that regular global variables could have different values in different worlds, but there is currently no case where the system does this. # Motivation The reasons for this change are manifold: 1. The primary motivation is to permit Revise to redefine structs. This has been a feature request since the very begining of Revise (timholy/Revise.jl#18) and there have been numerous attempts over the past 7 years to address this, as well as countless duplicate feature request. A past attempt to implement the necessary julia support in #22721 failed because the consequences and semantics of re-defining bindings were not sufficiently worked out. One way to think of this implementation (at least with respect to types) is that it provides a well-grounded implementation of #22721. 2. A secondary motivation is to make `const`-redefinition no longer UB (although `const` redefinition will still have a significant performance penalty, so it is not recommended). See e.g. the full discussion in #54099. 3. Not currently implemented, but this mechanism can be used to re-compile code where bindings are introduced after the first compile, which is a common performance trap for new users (#53958). 4. Not currently implemented, but this mechanism can be used to clarify the semantics of bindings import and resolution to address issues like #14055. # Implementation In this PR: - `Binding` gets `min_world`/`max_world` fields like `CodeInstance` - Various lookup functions walk this linked list using the current task world_age as a key - Inference accumulates world bounds as it would for methods - Upon binding replacement, we walk all methods in the system, invalidating those whose uninferred IR references the replaced GlobalRef - One primary complication is that our IR definition permits `const` globals in value position, but if binding replacement is permitted, the validity of this may change after the fact. To address this, there is a helper in `Core.Compiler` that gets invoked in the type inference world and will rewrite the method source to be legal in all worlds. - A new `@world` macro can be used to access bindings from old world ages. This is used in printing for old objects. - The `const`-override behavior was changed to only be permitted at toplevel. The warnings about it being UB was removed. Of particular note, this PR does not include any mechanism for invalidating methods whose signatures were created using an old Binding (or types whose fields were the result of a binding evaluation). There was some discussion among the compiler team of whether such a mechanism should exist in base, but the consensus was that it should not. In particular, although uncommon, a pattern like: ``` f() = Any g(::f()) = 1 f() = Int ``` Does not redefine `g`. Thus to fully address the Revise issue, additional code will be required in Revise to track the dependency of various signatures and struct definitions on bindings. # Demo ``` julia> struct Foo a::Int end julia> g() = Foo(1) g (generic function with 1 method) julia> g() Foo(1) julia> f(::Foo) = 1 f (generic function with 1 method) julia> fold = Foo(1) Foo(1) julia> struct Foo a::Int b::Int end julia> g() ERROR: MethodError: no method matching Foo(::Int64) The type `Foo` exists, but no method is defined for this combination of argument types when trying to construct it. Closest candidates are: Foo(::Int64, ::Int64) @ Main REPL[6]:2 Foo(::Any, ::Any) @ Main REPL[6]:2 Stacktrace: [1] g() @ Main ./REPL[2]:1 [2] top-level scope @ REPL[7]:1 julia> f(::Foo) = 2 f (generic function with 2 methods) julia> methods(f) # 2 methods for generic function "f" from Main: [1] f(::Foo) @ REPL[8]:1 [2] f(::@world(Foo, 0:26898)) @ REPL[4]:1 julia> fold @world(Foo, 0:26898)(1) ``` # Performance consideration On my machine, the validation required upon binding replacement for the full system image takes about 200ms. With CedarSim loaded (I tried OmniPackage, but it's not working on master), this increases about 5x. That's a fair bit of compute, but not the end of the world. Still, Revise may have to batch its validation. There may also be opportunities for performance improvement by operating on the compressed representation directly. # Semantic TODO - [ ] Do we want to change the resolution time of bindings to (semantically) resolve them immediately? - [ ] Do we want to introduce guard bindings when inference assumes the absence of a binding? # Implementation TODO - [ ] Precompile re-validation - [ ] Various cleanups in the accessors - [ ] Invert the order of the binding linked list to make the most recent one always the head of the list - [ ] CodeInstances need forward edges for GlobalRefs not part of the uninferred code - [ ] Generated function support
This implements world-age partitioning of bindings as proposed in #40399. In effect, much like methods, the global view of bindings now depends on your currently executing world. This means that `const` bindings can now have different values in different worlds. In principle it also means that regular global variables could have different values in different worlds, but there is currently no case where the system does this. The reasons for this change are manifold: 1. The primary motivation is to permit Revise to redefine structs. This has been a feature request since the very begining of Revise (timholy/Revise.jl#18) and there have been numerous attempts over the past 7 years to address this, as well as countless duplicate feature request. A past attempt to implement the necessary julia support in #22721 failed because the consequences and semantics of re-defining bindings were not sufficiently worked out. One way to think of this implementation (at least with respect to types) is that it provides a well-grounded implementation of #22721. 2. A secondary motivation is to make `const`-redefinition no longer UB (although `const` redefinition will still have a significant performance penalty, so it is not recommended). See e.g. the full discussion in #54099. 3. Not currently implemented, but this mechanism can be used to re-compile code where bindings are introduced after the first compile, which is a common performance trap for new users (#53958). 4. Not currently implemented, but this mechanism can be used to clarify the semantics of bindings import and resolution to address issues like #14055. In this PR: - `Binding` gets `min_world`/`max_world` fields like `CodeInstance` - Various lookup functions walk this linked list using the current task world_age as a key - Inference accumulates world bounds as it would for methods - Upon binding replacement, we walk all methods in the system, invalidating those whose uninferred IR references the replaced GlobalRef - One primary complication is that our IR definition permits `const` globals in value position, but if binding replacement is permitted, the validity of this may change after the fact. To address this, there is a helper in `Core.Compiler` that gets invoked in the type inference world and will rewrite the method source to be legal in all worlds. - A new `@world` macro can be used to access bindings from old world ages. This is used in printing for old objects. - The `const`-override behavior was changed to only be permitted at toplevel. The warnings about it being UB was removed. Of particular note, this PR does not include any mechanism for invalidating methods whose signatures were created using an old Binding (or types whose fields were the result of a binding evaluation). There was some discussion among the compiler team of whether such a mechanism should exist in base, but the consensus was that it should not. In particular, although uncommon, a pattern like: ``` f() = Any g(::f()) = 1 f() = Int ``` Does not redefine `g`. Thus to fully address the Revise issue, additional code will be required in Revise to track the dependency of various signatures and struct definitions on bindings. ``` julia> struct Foo a::Int end julia> g() = Foo(1) g (generic function with 1 method) julia> g() Foo(1) julia> f(::Foo) = 1 f (generic function with 1 method) julia> fold = Foo(1) Foo(1) julia> struct Foo a::Int b::Int end julia> g() ERROR: MethodError: no method matching Foo(::Int64) The type `Foo` exists, but no method is defined for this combination of argument types when trying to construct it. Closest candidates are: Foo(::Int64, ::Int64) @ Main REPL[6]:2 Foo(::Any, ::Any) @ Main REPL[6]:2 Stacktrace: [1] g() @ Main ./REPL[2]:1 [2] top-level scope @ REPL[7]:1 julia> f(::Foo) = 2 f (generic function with 2 methods) julia> methods(f) [1] f(::Foo) @ REPL[8]:1 [2] f(::@world(Foo, 0:26898)) @ REPL[4]:1 julia> fold @world(Foo, 0:26898)(1) ``` On my machine, the validation required upon binding replacement for the full system image takes about 200ms. With CedarSim loaded (I tried OmniPackage, but it's not working on master), this increases about 5x. That's a fair bit of compute, but not the end of the world. Still, Revise may have to batch its validation. There may also be opportunities for performance improvement by operating on the compressed representation directly. - [ ] Do we want to change the resolution time of bindings to (semantically) resolve them immediately? - [ ] Do we want to introduce guard bindings when inference assumes the absence of a binding? - [ ] Precompile re-validation - [ ] Various cleanups in the accessors - [ ] Invert the order of the binding linked list to make the most recent one always the head of the list - [ ] CodeInstances need forward edges for GlobalRefs not part of the uninferred code - [ ] Generated function support
Since
struct
s cannot be redefined, attempting to do so currently gives afailure to evaluate changes
warning.I wonder if this can be worked around by triggering a reload of the entire module in this case.
The text was updated successfully, but these errors were encountered: