Recurse through structs to check is_traced
#931
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
lib/ReactantCore/src/ReactantCore.jl
Outdated
| if isprimitivetype(x) | ||
| return false | ||
| else | ||
| return any(is_traced, getfield.(Ref(x), fieldnames(T))) |
There was a problem hiding this comment.
does this properly deal with recursive types? I.e. a linked list node
There was a problem hiding this comment.
Not the first time I've made this mistake...
I'll add a seen IdSet
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
You also need to bump the ReactantCore version number, and require that in Reactant
There was a problem hiding this comment.
Do I need to bump Reactant itself as well?
There was a problem hiding this comment.
Yes, so we can get a new release including this. Side note, in case you missed them, there are more suggestions above. Thanks!
There was a problem hiding this comment.
Side note, in case you missed them, #931 (review).
Yes, thanks a lot! Didn't wanna disturb CI yet.
I'm a bit puzzled about the one CI failure https://github.com/EnzymeAD/Reactant.jl/actions/runs/13898325850/job/38883950334?pr=931
There was a problem hiding this comment.
Oh, sadly the segfaults on the aarch64 runners are kinda expected: rust-lang/rust#135867. Restart those jobs if they only fail because of segfaults.
There was a problem hiding this comment.
ah, honestly great to hear 😝
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Mosè Giordano <[email protected]>
lib/ReactantCore/src/ReactantCore.jl
Outdated
| function is_traced((@nospecialize x::T), seen=(@static if VERSION > v"1.11" | ||
| IdSet() | ||
| else | ||
| Set{UInt}() |
There was a problem hiding this comment.
why can't we use Idset everywhere?
There was a problem hiding this comment.
doesn't exist in Julia 1.10 AFAICT
There was a problem hiding this comment.
can we define it? IdDict definitely exists for lower versions (though apparently not idset)
There was a problem hiding this comment.
Was it not exported before? I can do Base.IdSet in Julia v1.7
There was a problem hiding this comment.
Argh, will add, gimme a min
|
I'm going to merge this, so we can get the Ninner PR in soon. |
* Use concrete `Ninner` * Test EnzymeAD/Reactant.jl#931 * Test CliMA/Oceananigans.jl#4226 * Make Ninner concrete also for run script * add updated ReactantCore * Increase Ninner, see if that affects compile time * Revert "Increase Ninner, see if that affects compile time" This reverts commit 3abd73d. The test was successful. * Switch back to stable Reactant * Update CompileOrRun.yml * Update CompileOrRun.yml * Use stable version of all packages --------- Co-authored-by: jumerckx <[email protected]>
fixes #929