Skip to content

Conversation

@avik-pal
Copy link
Collaborator

@avik-pal avik-pal commented Dec 15, 2024

Cross posting from https://prontolab.slack.com/archives/C07JU2SCP9C/p1734235298670169 for details

Any idea how to make the reactant_override stuff work nicely with Revise. currently if we make any arbitrary change in a file containing a reactant_override it fails to revise. For example, adding:

foooooo(sss) = sss

before the overrides for Enzyme leads to

stmt = :($(Expr(:method, :(Base.getproperty(Reactant, :REACTANT_METHOD_TABLE)), %J11, CodeInfo(
1 ─      nothing
│   @ /mnt/software/lux/Reactant.jl/src/Interpreter.jl:486 within `none`
│   %2 = overload_autodiff
│   %3 = Core.tuple(rmode, f, rt)
│   %4 = Core._apply_iterate(Base.iterate, %2, %3, args)
└──      return %4
))))
┌ Error: Failed to revise /mnt/software/lux/Reactant.jl/src/Interpreter.jl
│   exception =
│    name Expr not recognized
│    Stacktrace:
│     [1] error(::String, ::Type, ::String)
│       @ Base ./error.jl:44
│     [2] direct_links!(cl::LoweredCodeUtils.CodeLinks, src::Core.CodeInfo)
│       @ LoweredCodeUtils /mnt/.julia/packages/LoweredCodeUtils/RTbGF/src/codeedges.jl:246
└ @ Revise /mnt/.julia/packages/Revise/vhmOR/src/packagedef.jl:743
┌ Warning: The running code does not match the saved version for the following files:
│ 
│   /mnt/software/lux/Reactant.jl/src/Interpreter.jl
│ 
│ If the error was due to evaluation order, it can sometimes be resolved by calling `Revise.retry()`.
│ Use Revise.errors() to report errors again. Only the first error in each file is shown.
│ Your prompt color may be yellow until the errors are resolved.
└ @ Revise /mnt/.julia/packages/Revise/vhmOR/src/packagedef.jl:854

If there is no nice solution, does anyone mind if I move all the overlays to a separate file "Overrides.jl"? That way Revise only doesn't work if a change is made to that particular file

@avik-pal avik-pal force-pushed the ap/overrides branch 2 times, most recently from a05ba91 to 2120d3b Compare December 15, 2024 09:36
@mofeing
Copy link
Collaborator

mofeing commented Dec 15, 2024

I like this.

The only thing left is that if we want to rename @reactant_override to @reactant_overlay to keep consistency with Julia (in the end it's just a partial to @overlay) this is a fantastic moment to do it. If not, let's keep then with "override".

@mofeing
Copy link
Collaborator

mofeing commented Dec 15, 2024

@avik-pal what is the difference between "TracedUtils.jl" and "Tracing.jl"? i created "Tracing.jl" to put all things related to tracing but not TracedRArray, and as i see, "TracedRUtils.jl" has things related to "TracedRArray" (like materialize_traced_array) but also not related to them like make_mlir_fn.

should i put a comment on top of the files to tell what that file is about? like i did in "Ops.jl"

@wsmoses
Copy link
Member

wsmoses commented Dec 15, 2024

@avik-pal what is the difference between "TracedUtils.jl" and "Tracing.jl"? i created "Tracing.jl" to put all things related to tracing but not TracedRArray, and as i see, "TracedRUtils.jl" has things related to "TracedRArray" (like materialize_traced_array) but also not related to them like make_mlir_fn.

should i put a comment on top of the files to tell what that file is about? like i did in "Ops.jl"

I needed a namespace such that we told our interpreter we don't need to continue recursing into it and replacing all code with code from our interpreter (for compile time performance). Ops is one such namespace, as is TracedUtils

@mofeing
Copy link
Collaborator

mofeing commented Dec 15, 2024

I needed a namespace such that we told our interpreter we don't need to continue recursing into it and replacing all code with code from our interpreter (for compile time performance). Ops is one such namespace, as is TracedUtils

aha and the same thing for TracedRArrayOverrides and TracedRNumberOverrides? although they don't seem like they are considered in should_rewrite_ft, so maybe no.

i don't like much that TracedRArray and TracedRNumber are back again in "Reactant.jl" file, but i guess this is one of the things you said about "let's get it done first and clean it later" right? i think that it makes sense to move "TracedUtils.jl" inside "Tracing.jl" and put it all inside a Tracing module. i'm ok waiting you to finish and then cleaning.

@wsmoses
Copy link
Member

wsmoses commented Dec 15, 2024

Not quite, those were there because I wanted to avoid exporting any internal utilities for implementing the traced overrides of Base

And yeah definitely go for reorganizing things [I just wanted to get things running to start with]

@avik-pal
Copy link
Collaborator Author

Just to confirm this is good to merge right? We can do reorganization in a later PR

@mofeing
Copy link
Collaborator

mofeing commented Dec 16, 2024

Not quite, those were there because I wanted to avoid exporting any internal utilities for implementing the traced overrides of Base

And yeah definitely go for reorganizing things [I just wanted to get things running to start with]

yeah, will still wait for you to finish so there are no overlaps or conflicts

@mofeing
Copy link
Collaborator

mofeing commented Dec 16, 2024

all good from my side

just be careful with @wsmoses's #385

@avik-pal avik-pal merged commit a98c02b into main Dec 16, 2024
28 of 50 checks passed
@avik-pal avik-pal deleted the ap/overrides branch December 16, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants