Repository navigation
Conversation
| else: | ||
| # Set was maybe never called, so still check it here | ||
| if is_torchdynamo_compiling(): | ||
| self.is_compiling = True |
There was a problem hiding this comment.
Side note: This was even a typo 👀 self.compiling <-> self.is_compiling
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
I was actually always checking like this in the beginning, then introduced the flag but I cannot remember if there was a specific reason test-wise... 😅 I guess if the test_generate_compile_model_forward_fullgraph and test_generate_compilation_all_outputs all pass, then it's good!
EDIT: Removed my approval as the CI actually did not trigger those tests at all (test fetcher will not run any model test if only output_capturing is modified I believe)! Will add the file to the all-trigger list actually, as it's an important feature that will soon touch all files with your coming PR!
| self.global_var = value | ||
| # Same, check the current state dynamically rather than relying on cached attributes |
There was a problem hiding this comment.
Why do we always set the global here? Let's only do when it's compiling!
There was a problem hiding this comment.
This is essentially the core change, it seems somewhere along the way, some small graph break happens and when we do not set it in general the state between compiled regions and non-compiled eager regions is messed up
There was a problem hiding this comment.
This is concerning, it means some modules are fully skipped by dynamo and not compiled??
| # NOTE: Local attributes/flags may be interpreted once during tracing | ||
| # and effectively treated as constant across compiled frames. If we | ||
| # cached such a flag, eager frames could incorrectly fall back to the | ||
| # shared global value, breaking visibility between eager wrappers and | ||
| # compiled hooks |
There was a problem hiding this comment.
I don't really understand this comment (the final part), so not helpful IMO haha - let's either entirely remove or rewrite!
There was a problem hiding this comment.
Will update in a second
| self.global_var = None | ||
| self.compiling = False | ||
| else: | ||
| self.global_var = self.default |
There was a problem hiding this comment.
Let's just reset to None alway no? Instead of saving the default as internal attr
There was a problem hiding this comment.
I don't mind, but why do we even provide a default in the init at all 😅
There was a problem hiding this comment.
Only because ContextVar requires it! Though indeed we could simply explicitly do it in our wrapper's init instead of exposing it
There was a problem hiding this comment.
Yea changed the init, and forced none in any case
Oh wait actually the tests did not trigger!
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=44614&sha=04569d |
|
Superseded by #44638 after more investigation! |
Only detected in #43590, so it can only be detected there for
test_sdpa_can_compile_dynamic(lw_detr). Core issue: Dynamo can cache the attribute and ignore it across frames which means that updates/reads are not working as expected.This PR changes the logic to force dynamo to read compile state instead of relying on the attribute
Note
Medium Risk
Changes how output-capture state is stored/read during
torch.compile, which can affect correctness in compiled vs eager regions and has limited thread-safety while compiling. Also broadens CI triggering by marking the file as core, potentially increasing test load but reducing missed regressions.Overview
Fixes output capturing under
torch.compileby reworkingCompileableContextVarto always query Dynamo’s current compiling state and to keep a mirroredglobal_varin sync, avoiding stale cached flags across graph breaks.Updates CI test selection by adding
src/transformers/utils/output_capturing.pytoCORE_FILES, so changes to output capturing trigger full test runs.Written by Cursor Bugbot for commit 04569d0. This will update automatically on new commits. Configure here.