Skip to content
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

variable scope of @thunk #684

Open
chengchingwen opened this issue Feb 8, 2025 · 0 comments · May be fixed by #686
Open

variable scope of @thunk #684

chengchingwen opened this issue Feb 8, 2025 · 0 comments · May be fixed by #686

Comments

@chengchingwen
Copy link

#683 introduces a separate path when creating the thunk (_usethunks() ? Thunk($(esc(func))) : $(esc(body))). However, this would introduce new variables into the caller's scope and create Core.Box thus causing type instability, which breaks some rrule type stability checks in NeuralAttentionlib.jl.

Currently, I have two ways to solve this:

  1. replace $(esc(body)) with $(esc(func))() which turns the else path into a function call, thus no variable introduction.
  2. mimic how @async and @spawn using $ to interpolate values. so @thunk would be defined as:
macro thunk(body)
    letargs = Base._lift_one_interp!(body)
    func = Base.replace_linenums!(:(()->($(esc(body)))), __source__)
    return quote
        if _usethunks()
            let $(letargs...)
                Thunk($func)
            end
        else
            let $(letargs...)
                $(esc(body))
            end
        end
    end
end

However, I don't understand #683 enough to see what would be better for 2nd order AD. Maybe @pxl-th or @mcabbott would have some thoughts on this.

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 a pull request may close this issue.

1 participant