[HigherOrderOp] Remove _deprecated_global_ns from cond #104380
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/104380
Note: Links to docs will display an error until the docs builds have been completed. ✅ 1 Unrelated FailureAs of commit d04d2e2: BROKEN TRUNK - The following job failed but were present on the merge base b073f6a:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@ydwu4 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@ydwu4 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was exported from Phabricator. Differential Revision: D47110919 |
torch/fx/node.py
Outdated
There was a problem hiding this comment.
Does this also change map? If so, that may be a problem, because map is still torch.ops.map.
The check isn't too robust:
- Ideally cond's
__name__is "cond" and__module__is "torch.ops.higher_order". (Is this the case?) - If so, then we should return
f"{func.__module__}.{func.__name__}
This also makes it a bit more future-proof (if we allow people to add higher order ops under other namespaces in the future.
There was a problem hiding this comment.
This pr doesn't deal with map. My plan is to experiment on cond a bit to see how it goes then do it for map in the next pr.
- cond's __name__ is "cond" but __module__ is "torch._ops". I can check a bit how to make its __module__ torch.ops.higher_order.
- Yeah, I agree.
|
@ydwu4 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| # Following _OpNamespace.__getattr__, we cache the op on the _PyOpNamespace object. | ||
| op = self._ops.get(name, None) | ||
| if op is None: | ||
| raise AttributeError( | ||
| f"'_PyOpNamespace' '{self.name}' object has no attribute '{name}'" | ||
| ) | ||
| setattr(self, name, op) | ||
| return op |
There was a problem hiding this comment.
Is this necessary for this PR? Or is it just an optimization?
There was a problem hiding this comment.
yeah, it's just an optimization. I was looking at how other opearator's __module__ is set and found this logic and the error message seems pretty good. So I "copied" it over.
There was a problem hiding this comment.
Sounds good, thanks for explaining
zou3519
left a comment
zou3519
left a comment
There was a problem hiding this comment.
LGTM assuming the optimization isn't necessary for this PR. If it is then I am probably misunderstanding something
|
This pull request was exported from Phabricator. Differential Revision: D47110919 |
Summary: Remove _deprecated_global_ns from cond following pytorch#104105. To do that, we need to change how graph_module generates python_code for "cond" target. Otherwise, it will generate target as "torch.ops.cond", which is invalid after the change. Will import this PR to fix internal tests. cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 Pull Request resolved: pytorch#104380 Reviewed By: zou3519 Differential Revision: D47110919 Pulled By: ydwu4 fbshipit-source-id: e0b8090982cee2c1415abb663bb6308eeb4a52b4
|
This pull request was exported from Phabricator. Differential Revision: D47110919 |
Summary: Remove _deprecated_global_ns from cond following pytorch/pytorch#104105. To do that, we need to change how graph_module generates python_code for "cond" target. Otherwise, it will generate target as "torch.ops.cond", which is invalid after the change. Will import this PR to fix internal tests. cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 X-link: pytorch/pytorch#104380 Reviewed By: zou3519 Differential Revision: D47110919 Pulled By: ydwu4 fbshipit-source-id: eed2f7e0aa6bfc0d0a46f0064630abed872e3d75
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
5 participants
Remove _deprecated_global_ns from cond following #104105.
We change the module attribute of HigherOrderOperator instances in the constructor from torch.ops to torch.ops.higher_order when self.namespace is "higher_order". For subclasses (e.g. customized higher order operator), we leave their __module__ unchanged.
Will import this PR to fix internal tests.
cc @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @ipiszy @chenyang78