-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Set default value for nccl make MAX_JOBS if ProcessorCount returns 0 #84231
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
land after #84209 [ghstack-poisoned]
🔗 Helpful links
✅ No Failures (0 Pending)As of commit 0c57d84 (more details on the Dr. CI page): Expand to see more💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
The error message I got is:
|
… returns 0" land after #84209 [ghstack-poisoned]
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.
LGTM if CI is green (why env
is needed though)?
cmake/External/nccl.cmake
Outdated
@@ -38,6 +42,7 @@ if(NOT __NCCL_INCLUDED) | |||
BUILD_IN_SOURCE 1 | |||
CONFIGURE_COMMAND "" | |||
BUILD_COMMAND | |||
env |
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.
Why this is needed?
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.
It was there before, but removed by #83696. I thought we need to pass this to pass env vars to make if necessary?
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.
No env
vars are being passed though, it was left over from the CCACHE_DISABLE=1
that was there before.
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.
Oh I see, let me remove that then.
cmake/External/nccl.cmake
Outdated
@@ -38,6 +42,7 @@ if(NOT __NCCL_INCLUDED) | |||
BUILD_IN_SOURCE 1 | |||
CONFIGURE_COMMAND "" | |||
BUILD_COMMAND | |||
env |
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.
is this change intentional?
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.
Yep, it was there before, but removed by #83696. I thought we need to pass this to pass env vars to make
if necessary?
… returns 0" land after #84209 [ghstack-poisoned]
@pytorchbot merge |
@pytorchbot successfully started a merge job. Check the current status here and land check progress here. |
…84231) Pull Request resolved: #84231 Approved by: https://github.com/malfet, https://github.com/rohan-varma
Merge failedReason: 2 additional jobs have failed, first few of them are: trunk ,trunk / macos-12-py3-arm64 / test (default, 1, 2, macos-m1-12) If you believe this is an error, you can use the old behavior with Please reach out to the PyTorch DevX Team with feedback or questions! Details for Dev Infra teamRaised by workflow job |
@pytorchbot merge -g |
1 similar comment
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here. |
Hey @mrshenli. |
…84231) (#84231) Summary: Pull Request resolved: #84231 Approved by: https://github.com/malfet, https://github.com/rohan-varma Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/56a37ea1a6e89a8aa31abc888127ccac647b92d4 Reviewed By: mehtanirav Differential Revision: D39142407 Pulled By: mrshenli fbshipit-source-id: 2ee44484605ca3ef18fcbca37829c4727988dab7
Stack from ghstack (oldest at bottom):