🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ You can merge normally! (2 Unrelated Failures)

As of commit 84058f6 with merge base 452e1a7 ():

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

No need to use template. You can just extend the allreduce_impl function to take an argument with default value:
In ProcessGroupNCCL.hpp:

  c10::intrusive_ptr<Work> allreduce_impl(
      at::Tensor& tensor,
      const AllreduceOptions& opts = AllreduceOptions(),
      bool isBarrier = false);

@fduwjj @c-p-i-o Would a change of profiling title impacts Flight Recorder's analysis?

No need to use template. You can just extend the allreduce_impl function to take an argument with default value: In ProcessGroupNCCL.hpp:

  c10::intrusive_ptr<Work> allreduce_impl(
      at::Tensor& tensor,
      const AllreduceOptions& opts = AllreduceOptions(),
      bool isBarrier = false);

Thank you for the review.
There's already an optional argument opts with default value.
c10::intrusive_ptr<Work> allreduce_impl(at::Tensor& tensor, const AllreduceOptions& opts = AllreduceOptions());
auto work = allreduce_impl<true>(barrierTensor); here just takes the default opts
Not sure if adding the second optional argument would be good.

I am not sure if C++ does not support multiple optional arguments.
If specifying true / false is easier, we can put isBarrier as the first optional argument, and AllreduceOptions the second.
Then it would be valid to call: allreduce_impl(barrierTensor, true).

Using template here doubles the binary size for a very minor difference, that's why I would prefer argument form.

I am not sure if C++ does not support multiple optional arguments. If specifying true / false is easier, we can put isBarrier as the first optional argument, and AllreduceOptions the second. Then it would be valid to call: allreduce_impl(barrierTensor, true).

Using template here doubles the binary size for a very minor difference, that's why I would prefer argument form.

Discussed offline, to apply an extra "profiling name" input, and order it as the first optional input

@pytorchbot merge

Merge started

Your 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