Skip to content
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

Make TorchElastic timer importable on Windows #88522

Closed
wants to merge 4 commits into from

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Nov 4, 2022

Also, add torch.distributed to test imports, so that we would not
regress in the future

Fixes #85427

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 4, 2022

🔗 Helpful Links

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

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

✅ No Failures

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

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

Copy link
Collaborator

@d4l3k d4l3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like from torch.distributed.rpc is problematic on windows

======================================================================
ERROR [0.039s]: test_circular_dependencies (__main__.TestImports)
Checks that all modules inside torch can be imported
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\actions-runner\_work\pytorch\pytorch\test\test_testing.py", line 1809, in test_circular_dependencies
    mod = importlib.import_module(mod_name)
  File "C:\Jenkins\Miniconda3\lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "C:\actions-runner\_work\pytorch\pytorch\build\win_tmp\build\torch\distributed\optim\optimizer.py", line 9, in <module>
    from torch.distributed.rpc import RRef
ImportError: cannot import name 'RRef' from 'torch.distributed.rpc' (C:\actions-runner\_work\pytorch\pytorch\build\win_tmp\build\torch\distributed\rpc\__init__.py)

LGTM from the elastic side though

malfet and others added 3 commits November 10, 2022 06:08
Also, add `torch.distribtued` to test imports, so that we would not
regress in the future
@malfet malfet force-pushed the malfet/make-elastic-importable-on-windows branch from 0a28c08 to a7146d5 Compare November 10, 2022 14:08
@malfet
Copy link
Contributor Author

malfet commented Nov 10, 2022

@pytorchbot merge -f "Win tests are green"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@malfet
Copy link
Contributor Author

malfet commented Nov 10, 2022

Looks like from torch.distributed.rpc is problematic on windows

@d4l3k yeah, had to exclude a bunch of subpackages in torch.distribtued. as RPC is not compiled for Windows for some reason, see:

if(USE_DISTRIBUTED)
append_filelist("libtorch_distributed_base_sources" TORCH_SRCS)
if(NOT WIN32)
append_filelist("libtorch_distributed_extra_sources" TORCH_SRCS)
endif()
endif()

@malfet malfet deleted the malfet/make-elastic-importable-on-windows branch November 10, 2022 18:08
izaitsevfb pushed a commit to izaitsevfb/pytorch that referenced this pull request Dec 2, 2022
Also, add `torch.distributed` to test imports, so that we would not
regress in the future

Fixes pytorch#85427
Pull Request resolved: pytorch#88522
Approved by: https://github.com/d4l3k

(cherry picked from commit f98edfc)
izaitsevfb pushed a commit to izaitsevfb/pytorch that referenced this pull request Dec 6, 2022
Also, add `torch.distributed` to test imports, so that we would not
regress in the future

Fixes pytorch#85427
Pull Request resolved: pytorch#88522
Approved by: https://github.com/d4l3k

(cherry picked from commit f98edfc)
atalman pushed a commit that referenced this pull request Dec 6, 2022
Also, add `torch.distributed` to test imports, so that we would not
regress in the future

Fixes #85427
Pull Request resolved: #88522
Approved by: https://github.com/d4l3k

(cherry picked from commit f98edfc)

Co-authored-by: Nikita Shulga <nshulga@meta.com>
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
Also, add `torch.distributed` to test imports, so that we would not
regress in the future

Fixes pytorch#85427
Pull Request resolved: pytorch#88522
Approved by: https://github.com/d4l3k
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

torchrun AttributeError caused by file_based_local_timer on Windows
3 participants