Import utils compilation fixes - #47726
Conversation
|
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. |
| @torch._dynamo.allow_in_graph | ||
| def _sonicmoe_wrapper( |
There was a problem hiding this comment.
dropping this since we removed the "not torch compiling" assertions in huggingface/kernels-community#1056
vasqu
left a comment
vasqu
left a comment
There was a problem hiding this comment.
Got some questions first tbh, not opposed just want the full picture
| capability, an environment variable. Never for a runtime query such as `is_cuda_stream_capturing`, | ||
| where inlining a value that legitimately changes would silently bake a transient into the graph. | ||
| """ | ||
| fn._dynamo_marked_constant = True |
There was a problem hiding this comment.
- Should we use settatr instead then?
- Also let's move it a bit below so when the actual import utils start for each package
- this seems like something we want for most utils here then? at which boundary do we decide if it's important or not?
There was a problem hiding this comment.
Iiuc then it's mostly appearing in torch distributed where the calls are not friendly if not marked as constant?
There was a problem hiding this comment.
basically any util that might make it into the forward path, the problem is that in many of our tests / compilation scenarios, the util might have been called before being compiled so it gets cached and tests don't catch it (for example it gets called in prefill before being compiled for decode). but it can break when compiled from the get go.
There was a problem hiding this comment.
Lets move and use setattr pls
| return _is_package_available("smdistributed")[0] | ||
|
|
||
|
|
||
| @_compile_constant |
There was a problem hiding this comment.
For anything env related, it could also switch at runtime theoretically no?
There was a problem hiding this comment.
yeh but i guess you could say that when you compile, you should expect those values to freeze in the graph. even a library/package availability can change at runtime actually 🫠
There was a problem hiding this comment.
Hmm, but I then feel like maybe we should not make this a default and allow making it a constant when explicitly requested? It's a bit brittle because we don't know what users use which functionality outside of this and we go against bc behavior then
Any option we make this optional and mark it under our runtime requirements where needed?
There was a problem hiding this comment.
allow making it a constant when explicitly requested
it's only a constant when compiled tho, we don't make it a constant with this flag, we only make it assumed a constant by the compiler (the only way for it to compile).
There was a problem hiding this comment.
Ah sorry, yea the docstrings even mention it.. wouldnt it kind of make sense to attach to all utils we have in the file
There was a problem hiding this comment.
Only potentially not necessary ^
There was a problem hiding this comment.
adding it to as many as possible, but there are some that should be left without
| """ | ||
| These helpers get called from inside `torch.compile`d regions — e.g. `is_dtensor`, which every MoE | ||
| kernel integration reaches through `to_local`. Each carries `@_compile_constant`, so dynamo evaluates | ||
| it once at trace time and never enters the body; this checks the marker actually takes effect. |
There was a problem hiding this comment.
Yea exactly where it would be useful to know how to determine this list
vasqu
left a comment
vasqu
left a comment
There was a problem hiding this comment.
Ok sorry for all the back and forth, just small comments left imo
| capability, an environment variable. Never for a runtime query such as `is_cuda_stream_capturing`, | ||
| where inlining a value that legitimately changes would silently bake a transient into the graph. | ||
| """ | ||
| fn._dynamo_marked_constant = True |
There was a problem hiding this comment.
Lets move and use setattr pls
| return _is_package_available("smdistributed")[0] | ||
|
|
||
|
|
||
| @_compile_constant |
There was a problem hiding this comment.
Only potentially not necessary ^
CI recapDashboard: View test results in Grafana |
3 participants
What does this PR do?
Import utils are very sneaky and break compile (distributed util to_local breaks compile bcz of distributed torch check).
Code Agent Policy
The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. These often are low-quality, or fix extremely minor issues that occur rarely or never in practice.
As a result, we're instituting a rule that first-time contributors should not use code agents to submit PRs or issues.
We'd also ask autonomous "OpenClaw"-like agents not to open any PRs or issues.
Issues/PRs from first-time contributors that violate this rule will probably just be closed without review, and we
might block you, especially if you open more than one or appear to be deliberately ignoring this. We especially do not
want new contributors to jump in on random issues to contribute an agent-written fix. This creates lots of noise
for reviewers and other users and will almost certainly get you blocked.
For more information, please read
CONTRIBUTING.md.Before submitting
Pull Request checks?
to it if that's the case.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.