-
Notifications
You must be signed in to change notification settings - Fork 98
Support converting SymTypes Node to input proxy #2171
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
@IvanYashchuk would you like to review this? |
Hi @kshitij12345 @IvanYashchuk ,could you help to take a look |
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 @kiya00 , I just have one question.
Also, we should add a test, maybe something like the following. I think we should also verify that thunder caching works correctly and calling cfn
with different idx
value also returns the correct result.
import torch
from thunder.dynamo import thunderfx
def fn(x, idx):
return torch.select(x, 0, idx)
x = torch.randn(10, 10)
idx = 0
cfn = thunderfx(fn, dynamic=True)
cfn(x, idx)
assert cfn._backend.subgraph_infos[0].split_reasons == []
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, thanks @kiya00
Hi @IvanYashchuk @t-vi , could you help take a look, I think it's ready to merge |
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.
Looks good!
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.
Before submitting
What does this PR do?
While exploring the split reasons and fallback operators in issue #2169, it was found that the current implementation of
get_proxy_inputs_from_node
does not support SymInt types. It causes graph splits when running models likeUndi95/dbrx-base
andllama-moe/LLaMA-MoE-v1-3_5B-4_16
.