Skip to content

Conversation

@mrshenli
Copy link
Contributor

@mrshenli mrshenli commented Sep 8, 2022

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 8, 2022

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/84719

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures, 3 Pending

As of commit c80ce69:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the release notes: distributed (c10d) release notes category label Sep 8, 2022
@facebook-github-bot facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels Sep 8, 2022
mrshenli added a commit that referenced this pull request Sep 8, 2022
ghstack-source-id: 4374054
Pull Request resolved: #84719
@mrshenli
Copy link
Contributor Author

mrshenli commented Sep 9, 2022

Compared to #84655, the fix is not exposing anything in _spmd/__init__.py file, otherwise, the imported non-public APIs in comm_tensor.py will break test_public_bindings.py.

mrshenli added a commit that referenced this pull request Sep 9, 2022
ghstack-source-id: be38c0d
Pull Request resolved: #84719
@mrshenli mrshenli added ciflow/trunk Trigger trunk jobs on your pull request ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR labels Sep 9, 2022
@wanchaol
Copy link
Collaborator

wanchaol commented Sep 9, 2022

Compared to #84655, the fix is not exposing anything in _spmd/__init__.py file, otherwise, the imported non-public APIs in comm_tensor.py will break test_public_bindings.py.

hmmm I didn't get this fully, why test_public_bindings would be bothered for anything inside a private module?

@mrshenli
Copy link
Contributor Author

mrshenli commented Sep 9, 2022

hmmm I didn't get this fully, why test_public_bindings would be bothered for anything inside a private module?

Error message looks like this. Before this PR, experimental's __init__.py didn't expose anything to that test. However, if we expose CommTensor which imports those private features, it makes those private features visible to the test which complains about missing __all__ in the experimental package.

Full list:
# torch.fx.experimental.proxy_tensor.ProxySymDispatchMode:
  - Is NOT public: it is not inside the module's (`torch.fx.experimental.proxy_tensor`) `__all__`
  - Does look public: it does look public because it follows the rules from the doc above (does not start with `_` and has a proper `__module__`).
  - You can do either of these two things to fix this problem:
    - To make it public: add it from the modules's (`torch.fx.experimental.proxy_tensor`) `__all__`
    - To make it NOT look public: make its name start with `_`

@mrshenli
Copy link
Contributor Author

mrshenli commented Sep 9, 2022

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

Hey @mrshenli.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request Sep 9, 2022
Summary:
Pull Request resolved: #84719
Approved by: https://github.com/wanchaol

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/2211949513d6ff126f1caae4d817055ec9d3fab1

Reviewed By: izaitsevfb

Differential Revision: D39386513

Pulled By: izaitsevfb

fbshipit-source-id: 6e5949583429f02f5c92cad2605cd1292168381f
rdspring1 pushed a commit to rdspring1/pytorch that referenced this pull request Sep 10, 2022
@facebook-github-bot facebook-github-bot deleted the gh/mrshenli/343/head branch September 12, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR ciflow/trunk Trigger trunk jobs on your pull request cla signed Merged oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: distributed (c10d) release notes category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants