-
Notifications
You must be signed in to change notification settings - Fork 351
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
feat: exclude refit sensitive ops from TRT compilation #3159
Conversation
@@ -317,6 +318,10 @@ def compile_module( | |||
# Assume converters support dynamic shapes and disable validation | |||
CONVERTERS.set_dynamic_shape_support(settings.assume_dynamic_shape_support) | |||
|
|||
# Set non-refitable ops as disallowed targets. | |||
if settings.make_refitable: | |||
settings.torch_executed_ops.update(REFIT_SENSITIVE_OPS) |
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 think its better to do this through validators than maintaining another list and modifying user settings
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.
-
Currently, the capability validator doesn't have access to user settings. So, I think making the compilation settings in _compiler.py (compile call) a global variable and passing it here would work. https://github.com/pytorch/TensorRT/blob/main/py/torch_tensorrt/dynamo/conversion/_ConverterRegistry.py#L439
-
The other alternative if we don't want to modify user settings is
CONVERTERS.set_disallowed_targets(settings.REFIT_SENSITIVE_OPS)
What would be preferred ?
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.
Dont see a drawback to let validators see user settings, could be relevant later like if a certain converter only works for one data type
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.
So it would involve updating the converter registry to take the settings as an arg in addition to the node, and some adjustment in the decorator. But overall I think its the best design since we shouldnt be spreading this info in a bunch of global lists, a converter should be able to tell you what it can and cant do on its own
Dont think we need to make settings global we could make a reference a member of the registry like we do the dynamic shape setting
assume_dynamic_shape_support: bool = 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.
Just an additional arg here:
if candidate.capability_validator(node) and ( |
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.
Got it. modified it now.
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, make sure to mark needs cherrypick and ping @lanluo-nvidia
Description
make_refittable
is set toFalse
even after this addition.make_refittable=False
. This is because in the future, we wantmake_refitable=True
by default and hence these tests fail. So, explicitly marking these now will avoid those errors.Fixes https://github.com/pytorch/TensorRT/actions/runs/10637055400/job/29490744653?pr=3131
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: