-
Notifications
You must be signed in to change notification settings - Fork 0
[Issue #4403][refactor] Move pattern matching transforms to new InferenceOptimizer #98
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
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
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.
For any transform that we move we should:
- remove the corresponding transform from the old InferenceOptimizer
- configure the default settings in
auto_deploy/config/default.yaml
Once we have finalized the reviews, would you be comfortable submitting one PR per transform? (or at least one PR per a couple of transforms). This well help with tracking potential regressions in case we face any
|
||
# TODO:(hg) confirm this | ||
info = TransformInfo( | ||
skipped=False, num_matches=num_moe_patterns, is_clean=False, has_valid_shapes=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.
skipped=False, num_matches=num_moe_patterns, is_clean=False, has_valid_shapes=True | |
skipped=False, num_matches=num_moe_patterns, is_clean=False, has_valid_shapes=False |
This is safer unless we know that the transform correctly assigns and updates shapes.
@Fridah-nv can also comment on that
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 agree on this. I think all the transformations should be able to preserve valid shapes except for those using torch._inductor pattern matcher.
Should we require the other transformations to preserve it to avoid running shape propagation multiple times? cc: @lucaslie
|
||
# TODO:(hg) confirm this | ||
info = TransformInfo( | ||
skipped=False, num_matches=fused_key_counter, is_clean=False, has_valid_shapes=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.
skipped=False, num_matches=fused_key_counter, is_clean=False, has_valid_shapes=True | |
skipped=False, num_matches=fused_key_counter, is_clean=False, has_valid_shapes=False |
@Fridah-nv can also comment on this
|
||
# TODO:(hg) confirm this | ||
info = TransformInfo( | ||
skipped=False, num_matches=num_matches, is_clean=False, has_valid_shapes=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.
@Fridah-nv please help confirm @h-guo18 as well
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.
has_valid_shapes
should be set to False since the pattern matcher utility won't preserve shape information correctly
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
Signed-off-by: haoguo <[email protected]>
GH Issue #4403 [refactor] Move pattern matching transforms to new InferenceOptimizer
Description
Test Coverage
Unite tests. See changed files.