-
Notifications
You must be signed in to change notification settings - Fork 31.6k
Allow override inputs to export recipe #37508
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
Conversation
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the |
| dynamic_shapes = ( | ||
| dynamic_shapes | ||
| if dynamic_shapes is not None | ||
| else {"input_ids": {1: torch.export.Dim.AUTO}, "cache_position": None} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain a bit what is the value like torch.export.Dim.AUTO means, and the one in this guide
# Create a dynamic batch size
batch = Dim("batch")
# Specify that the first dimension of each input is that batch size
dynamic_shapes = {"x1": {0: batch}, "x2": {0: batch}}
IIRC, the key 0 or 1 means which dimension, but don't know torch.export.Dim.AUTO or Dim("batch").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key 0 or 1 means which dimension
Correct.
torch.export.Dim.AUTO or Dim("batch")
It's new feature introduced since 2.6.0, smarter than the traditional way of specifying the dynamic range, e.g Dim("seq_len", min, max). @tugsbayasgalan can explain more. @ydshieh if you think Dim("seq_len", min, max) is easier to understand, we can switch to explicit dynamism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can find a doc that explains what torch.export.Dim.AUTO does and why it's smarter, adding the link along with a a short comment in the source code here, that would be nice.
When I look doc or source, I can't find any info.
But if you can't find neither, it's fine. In this case, maybe we can reflect this situation to pytorch team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave it for @tugsbayasgalan to chime in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dim.AUTO works by refining the range as you encounter shape related constraints (possibly specializing to static integer). So in that sense, Dim.AUTO is less likely to run into shape related errors and doesn't require user code change as it will just respect user code intention. If you really want to keep seq_len to be dynamic, you should use Dim.DYNAMIC which will error when we specialize. For official doc, cc @pianpwk
SunMarc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks ! left a few comments
|
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. |
|
Other than the nit comments I left, just a question: so this dynamism is necessary if the exported model want to be used to call prompts (i.e. prefill stage) as well as to use the generation steps (here seq=1)? |
10e7489 to
772606e
Compare
tests/utils/test_cache_utils.py
Outdated
| input_ids = tokenizer("Here's everything I know", return_tensors="pt").input_ids | ||
| dynamic_shapes = {"input_ids": {1: torch.export.Dim.AUTO}, "cache_position": None} | ||
| exported_program = convert_and_export_with_cache( | ||
| model, example_input_ids=input_ids, dynamic_shapes=dynamic_shapes | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm export this model with a non-specialized input_ids and the dynamic shape on "seq_len" dim. @tugsbayasgalan Is there a way I can assert the exported program do have the dim set to be dynamic, and inspect what the dynamic range is?
772606e to
1451ea8
Compare
| if dynamic_shapes is not None: | ||
| logging.warning("Dynamic shapes spec will be ingored for < 2.6.0.") | ||
| if strict is not None: | ||
| logging.warning("strict flag spec will be ingored for < 2.6.0.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, first, I think we should have
if is_torch_greater_or_equal("2.6.0"):
...
elif is_torch_greater_or_equal("2.5.0"):
...
else:
...
right ...?
If so, the 2 new messages have to be in both elif and else.
Also, let's rephrase it as
Dynamic shapes spec will be ignored by
convert_and_export_with_cachefor < 2.6.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ydshieh That could be one option. Alternatively, we can simplify it by just not use dynamic shapes for torch<2.6 including 2.5, which is BC.
Additionally, in torch 2.5 though it supports dynamic shapes, but no Dim.AUTO. As Tugsuu mentioned above, w/o Dim.AUTO it is more likely to run into shape related errors. Hence I'd rather keep things BC by not using dynamic shapes for torch<2.6. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK :-)
|
just a if/else v.s. if/elif/else question and 2 nits, but overall LGTM. |
| if strict is not None: | ||
| logging.warning("strict flag spec will be ingored for < 2.6.0.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's default strict to True. Does it require 2.6 to set strict to False ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong option to default True or False, but I think the export team wants to promote default to False in newer version of torch. cc: @tugsbayasgalan
Shared my thoughts here: #37508 (comment). Just in case you missed it. Let me know you thoughts |
1451ea8 to
174c382
Compare
|
More comments on this PR? Can we merge it if no? |
|
I will rebase this PR since #37728 has been merged |
5390845 to
c2bd31f
Compare
ydshieh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the iterations. LGTM, torched test is passing and fast to run 👍
I will wait until the end of the day before merge to see if @SunMarc has any comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! Please fix the conflits and we are good to merge
bee41b7 to
cbf2207
Compare
|
Thanks |
Add option to specify dynamic shapes during export Co-authored-by: Guang Yang <[email protected]>
What does this PR do?
Enable dynamism at seq_len dim in order to utilize parallel
prefillin the executorch runtime. In this PR,torch.exportHybirdCacheTests
pytest tests/utils/test_cache_utils.py -vv -s -k cache_exportabilityBefore submitting
Pull Request section?
to it if that's the case. Needed by the downstream in optimum-executorch. Add export dynamic shape for causal lm optimum-executorch#53
documentation guidelines, and
here are tips on formatting docstrings.
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.
@ArthurZucker @ydshieh
CC: @tugsbayasgalan