-
Notifications
You must be signed in to change notification settings - Fork 350
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
[FX] refactor the fx path in compile function #1141
Conversation
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
Other general comments:
|
) | ||
if self.lower_setting.explicit_batch_dimension | ||
if self.lower_setting.explicit_batch_dimension and self.lower_setting.dynamic_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.
dynamic_batch is added to differentiate two cases: with or w/o dynamic shape on batch dim (dim=0). cc @wushirong. I keep the dynamic_batch=True as default value so it will not change the previous behavior in production. Please have a review.
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.
What's the use case of w/o dyanmic shape on batch dim with explicit_batch_dimension=True? What's the different in terms of behavior in TensorRT? Basically, if I have explicit_batch_dimension=True while all my input dims are positive, how does TRT interprets it?
Maybe a question to @narendasan too.
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.
Here is my understanding, firstly, explicit_batch_dimension=True will become the default in next year and there is no explicit_batch_dimension=False(implicit) mode.
Secondly,
if I have explicit_batch_dimension=True while all my input dims are positive, how does TRT interprets it?
TRT will treat it as fixed shape for any future input. And that is what I tested for all the torchdynamo benchmarks
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
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.
Add a description for the new field 'dynamic_batch', and I am a bit unsure whether we should set it default to true.
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
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.
Code conforms to C++ style guidelines
addressed and set it to True as default. |
Description
Separate the FX lowering process to a single function
compile
to align with existing ts compile.Type of change
Please delete options that are not relevant and/or add your own.
Checklist: