so.. this seems better than the hacky fqn/mangled name thing. But, is it totally reliable? I wondered if there could be edge cases, or if dynamo would possibly make copies, etc.

cc @voznesenskym @zdevito

if these parameters are id stable, why not just annotate them directly? at the ddp level, whenever I add to parameters_to_ignore it should be something like:

def mark_parameter_as_ignored(module, name):
    assert name in module.named_parameters()
    ignored_parameter_list.append(name)
    parameter = module.named_parameters()[name]
    parameter._ignored = True

And then you don't need to leak your bookeeping of ignored_parameter_list anywhere else (You could even get rid of it, potentially).

And in dynamo, you would just do:

p.requires_grad and not p._ignored:

Thanks @voznesenskym, i think that's a great idea

cc @aazzolini @mrshenli any issues with this approach?

SGTM. This is also what we are proposing for the new annotation-based API: https://fb.quip.com/bpvPA6f2dtrA

The only thing is that, we might want this at parameter level (instead of module-level) for DDP to have parity.

The only thing is that, we might want this at parameter level (instead of module-level) for DDP to have parity.

I thought it already was at the parameter level? See my latest code, i think it's what you want.

But now i'm confused- in DDPOptimizer I simply ignore all buffers, since i thought the implication was they never require grad, and thus wouldn't be allreduced by DDP. If some buffers get allreduced by DDP, then i'd want to follow this up with another PR that tests buffers and gets that behavior right.

For now i've marked both params/buffers that are in the parameters_and_buffers_to_ignore list with the same marker on the DDP side, since that seems consistent with the convention there.

@albanD says we can't rely on parameter id's being stable, that mostly works, but there are a few edge cases where it doesn't. In particular, reparametrization cannot always preserve original parameter id.

what does 'reparametrization' mean exactly?

i'm thinking it might be worth sticking with the current approach (marking params) as it is simple, and the consequences of getting it wrong are relatively minor (graph-breaks wouldn't exactly match ddp's buckets, so perf would degrade anywhere from a little bit to matching dynamo+ddp without graphbreaks.

but if there is another scheme that is not too complex i'd be open to it

Reparametrization is when you register a rule to recompute parameter every time before it's used https://pytorch.org/tutorials/intermediate/parametrizations.html

ok. i think i want to propose that we just land this as is.

• i'm not sure any of the users of DDP's ignored parameters flag today are also using parametrizations
• it wouldn't be catastrophic if ignored_parameters was not honored in ddp optimizer. (that is the defacto today)
• we could potentially revisit this later

Also, I'm curious-does dynamo+AOT handle parametrization on its own currently?

@mrshenli i thought buffers by definition do not require grad, and therefore ddp ignores them by default?

if not i should update the logic in DDPOptimizer accordingly.

Buffers are by default broadcast right before the forward pass if broadcast_buffers=True
is passed to DDP constructor. This is true by default.
But then, if the buffer appear on the igore_parameters field, it's not part of the broadcast.
I think buffers shouldn't count for the purposes of splitting the model since we're not syncing them after the backward pass.

ok, this makes sense. then my change is ok- we mark them but we still ignore them in ddp optimizer.

Buffers are by default broadcast right before the forward pass if broadcast_buffers=True
is passed to DDP constructor. This is true by default.
But then, if the buffer appear on the igore_parameters field, it's not part of the broadcast.
I think buffers shouldn't count for the purposes of splitting the model since we're not syncing them after the backward pass.

do we still need this?

it is just useful for visualization purposes. see the debug output on the next PR in this stack. the buckets table is printed using this string.