Enable strict=false in aot_compiler #12872
Conversation
mcr229
requested review from
JacobSzwejbka,
lucylq,
swolchok,
larryliu0820,
digantdesai and
jackzhxng
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12872
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 3d19bcc with merge base 2c84f70 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
This PR needs a
|
| @@ -1 +1 @@ | |||
| ab43fe4bdf5ccd82897f0e982c451a0127bd175e | |||
| 2dccff7dcf56b0d168ebfd7ca08bdeca37273c56 | |||
There was a problem hiding this comment.
Nit: Split it in a different PT? And all the c10 changes with it?
There was a problem hiding this comment.
sorry a lot of the changes that you reviewed here are not related commits, they just somehow got pulled into this PR
| @@ -278,7 +278,7 @@ def transform_fn(x): | |||
| return x[0] | |||
|
|
|||
| quantized_model = quantize_model( | |||
| cast(torch.fx.GraphModule, aten_dialect.module()), | |||
| cast(torch.fx.GraphModule, aten_dialect.module()), # type: ignore[redundant-cast] | |||
| def __init__(self): | ||
| self.parent = {} | ||
|
|
||
| def find(self, x): |
There was a problem hiding this comment.
Nit: change the fn name to reflect mutation of the internal state?
|
|
||
| # Union overlaps into a single group | ||
| if len(partition) > 0: | ||
| dsj.find(partition[0]) |
There was a problem hiding this comment.
Nit
| dsj.find(partition[0]) | |
| _ = dsj.find(partition[0]) |
|
|
||
| for external_node in user_nodes: |
There was a problem hiding this comment.
Nice. Does this speed things up?
| # this likely needs to be combined with the process_remaining_nodes | ||
| # TODO: this currently doesn't work with _process_remaining_nodes so | ||
| # if a user provides grouped nodes with operatorsupport, then this will | ||
| # faile |
There was a problem hiding this comment.
| # faile | |
| # fail |
|
|
||
| return matched_nodes | ||
| return dsj.gen_groups() |
There was a problem hiding this comment.
How different is it from the base partitioner disjoint set creation? i.e. can we reuse that?
https://github.com/pytorch/pytorch/blob/f3913ea641d871f04fa2b6588a77f63efeeb9f10/torch/fx/passes/infra/partitioner.py#L78
There was a problem hiding this comment.
this is lighter weight. I don't have to do the dep checks, I think an underlying assumption i'm making here is that the groups proposed by the configs are valid(do not create cycles). As a result we can reduce the number of checks we have to do.
There were some issues with ic3/ci4 when strict=false, turning it on in aot compiler and seeing what throws.