does the approach in this PR assume that we can put a ++ in every place that we start coalescing?

i think this fails due to the fact that user-code can also start a coalescing group (e.g. from python side) and they will not bump the counter.

I was assuming that from the python side, the coalescing group will eventually call ProcessGroupNCCL::collectiveCoalesced function.
In this function, we already do:
seqCollective_++.

Does the python side not eventually call the mentioned function above?

I should mention that the intent here is to fix the two use cases:

1. all_gather of different outputTensor sizes automatically turns on coalescing and
2. reduce_scatter of different inputTensor_ sizes.

Whenever python calls a coalesced operation, the call (if I read the code correctly) makes it to collectiveCoalesced function.

I changed the code go increment seqCollective_ in ProcessGroupNCCL::collective operation once per coalesced collective. This way, it should cover all API calls into this area.

Does the python side not eventually call the mentioned function above?

Unfortunately there are 2 paths from python. For some of the python coalescing, it starts by calling 'start' and then issuing normal coalesced calls, then calling 'end'.
https://github.com/pytorch/pytorch/blob/main/torch/distributed/distributed_c10d.py#L2274

increment seqCollective_ in ProcessGroupNCCL::collective operation once per coalesced collective

iirc this is actually a change in semantics. We need to document what the semantics for the seq numbers are. One rationale is, we want one seq number per actual 'work' object == actual GPU kernel. Since coalescing groups logical operations together into one actual gpu kernel, we also create only one 'work' obj that we enqueue into the watchdog for monitoring. It may be confusing to increment the seqnum for each collective inside the coalescing group.

For P2P + flightrecorder i remember confronting this and adding a new 'op id' or something, which does increment on every op, but was separated from the seq number. The FR could then use actual seq num to match whole coalescing groups with each other.

hmm. one more possible wrinkle, since i am clearly trying to be annoying here, is we actually do use the 'collective' communicator when doing coalesced p2ps. In other words, i think it might have been intentional to bump the collective seq for coalp2p. Someone check me on this?

ref: we get a special nccl stream and nccl comm for p2p ops, but iirc we use the normal stream/comm for coalp2p same as we would use for collectives.

given this, what makes the most sense from a logging pov? clearly it will confuse users if we increment collective seq when we do p2p. otoh, if we are trying to do trace analysis to debug a hang, it might be important to note that this coalp2p happened sequentially after a collective on the same ncclcomm.