Should Plugins expose certain parameters through Trainer?

[awaelchli]

One example is find_unused_parameters=True/False flag in the Trainer.
Several users have recently asked how to set this flag, mainly because the default of this parameter changed from 1.1 to 1.2 and some users are now forced to toggle it.

The current way is to change it like so:

Trainer(plugins=[DDPPlugin(find_unused_parameters=True/False)])

Some users have expressed a desire to set it directly in the Trainer.

Advantages:

• Easy to change
• Can make it accelerator/training type agnostic
• Can be easily used through argparse
• Less hidden in documentation, more natural for user to first search in Trainer signature

Disadvantages:

• Adds more trainer flag(s)
• Internal parsing of these arguments and edge case handling is more complicated

[SeanNaren]

I am in favour of adding the trainer flag as it's a flag that users touch in general PyTorch quite a bit. models like iGPT will not run in DDP mode without this.

Just for adding fuel to the discussion, I do wonder the long term future of this particular flag, as I know atleast for FairScale they are trying to not use find_unused_parameters in their custom DDP as this is cumbersome and slow, requiring a traversal over the graph. FairScale implementations will probably be upstreamed and DDP modified to support some the sharding techniques natively, so we might see it go away in months. But all that being said, I think it's safer to expose this flag.

I vote against the general kwargs approach however (sending kwargs through the trainer into the plugins/all functions). I think that adds additional confusion that I'd rather not deal with (I think the HF datasets do a similar thing, it makes it tricky to trace what params are going where without digging into the code/checking the docs). We should make an explicit find_unused_parameters flag exposed to the trainer, and deal with any additional flags we add to the trainer on a case by case basis.

[SeanNaren]

I agree with your points which is what makes this a difficult choice!

Just to play devils advocate, where do we draw the lines? We have tpu_cores defined in the trainer, this is a specific accelerator argument. However how could we expect users to use TPUs without this flag exposed? I think this flag is instrumental in adoption; if we asked the users to instantiate the TPUPlugin and pass it through, i wonder how things would've played out.

Without this flag, I'm sure we'll see people adding their own flag to add the DDPPlugin + the find_unused_parameter as default because it's necessary for a few select models which is boilerplate. It would be nice for users not to do this.

[justusschock]

TPUs are kind of a special case, but yeah that's some other point for discussion where we could think about cleaning up our interface a bit.

And to where we draw the lines: In you case that's simple. We TPUs are basically an accelerator not plugins (I know that there are plugins bundled with it, but basically it still is an accelerator).

I know we reverted this some time ago, but are there any fundamental drawbacks of setting find_unused_parameters to True per default?

[awaelchli]

We changed the default back to find_unused_parameters=False because people reported performance issues (right @SeanNaren?) and also because it's False by default in pytorch too.

[SeanNaren]

Yup, it comes with a speed tradeoff as discussed in the docs under Forward Pass: https://pytorch.org/docs/stable/notes/ddp.html

For most cases, it will work out the box without finding unused parameters. But in some cases it will either hang, or crash due to the break down in the buckets.

I spoke to @justusschock and I could imagine a future where we set it back to True, and suggest in the docs under Optimization tricks to turn this off but I'd like to get numbers around what's the drop off in speed when disabling/enabling this flag, which will help make the decision!

[ericharper]

I'd like for this to be a trainer flag defaulted to False. There's tons of trainer flags and it's really important to be able to set this one to True when needed.

[carmocca]

Should it be a property of the model instead? Just as automatic_optimization

[awaelchli]

oh, I like it :)

[justusschock]

Not sure that's better because then we actually introduce hooks and properties that aren't valid for different runtime environments which in general might lead to issues and code duplication.

[carmocca]

But that's the same as if the Trainer has it. The question is if it depends more on the current trainer setup or on the model architecture

[justusschock]

You're right. That probably depends on the actual model. Since sometimes you cannot train a model without a certain flag activated (especially the really big ones), which would make it a model property and sometimes it's just to scale training, which would make it a trainer property.

[Queuecumber]

It looks like this is defaulted to true again, but it definitely should be defaulted to False for two related reasons

1. That's the default in pytorch (so you're breaking people's expectations by changing it)
2. That's good enough for most people's models

It's only if you're doing something weird that you really need this, and it causes a performance penalty, so please revert to defaulting it to False.

I can tell you as a heavy user of this flag, making it a model property is a good idea because the flag is only required by certain model architectures it has nothing to do with the training setup and everything to do with how the model code was written. Therefore making it a model property makes sense.

Put another way, given a specific implementation of a model it will either always need this flag for distributed training or it will never need this flag, so it can be determined by the person implementing the model and hard coded.

[justusschock]

We actually defaulted this to True by choice. The reason for this is, that it works for all models instantly whereas with this being False you error out on some other models. You can easily set this to False for speedup if it works for you, but our goal is to provide defaults that work for everyone (even if that means switching PyTorch defaults), but that's why we made it customisable.

See https://pytorch-lightning.readthedocs.io/en/latest/benchmarking/performance.html#when-using-ddp-set-find-unused-parameters-false for reference

[Queuecumber]

Yeah it "works" for any model but at a performance cost, I would say at most around 10% of people actually need this so you're saying it's ok for lightning to be slow by default for 90% of models? I can make a model that crashes if, say, amp is disabled will you change the default for that so that my model doesn't crash?

Also I wouldn't exactly call the current way to customize this particularly easy. I strongly recommend the model property, as I pointed out the necessity of this flag is a function of the model implementation and nothing else, with a False default value. I'm saying this as someone with a lot of experience with this particular flag.

[SeanNaren]

Thanks @Queuecumber (great name btw :P)

I agree with the sentiment here. I was initially hesitant to re-enable find unused parameters to true, as it comes with a large performance detriment (it seems in your experiments its performed quite poorly, but using a GPT model I've seen no noticeable difference, however thats a very small sample size of 1).

We recently merged #6982 which allows the plugins to expose their own plugin flags. We're planning on adding a ddp_find_unused_parameters_false flag to expose this to the Trainer (pl.Trainer(plugins='ddp_find_unused_parameters_false'). Would you be alright with this?

The other solution is to (yet again) swap the find_unused_parameters to False as default, and expose a ddp_find_unused_parameters_true flag for the 10% who need this flag enabled.

[Queuecumber]

I wonder if it makes sense to do a twitter poll or something to find out what people generally think of this flags default value, it may help to have some hard data on who wants this and then make a final decision rather than flipping its default every so often when someone vocalizes about it.

The proposed trainer flag makes sense, some easy way to toggle it would be nice. Of the proposals the one that appealed to me the most was the model property, since this flag is so tied to how the model is written, but anything that makes it easy to toggle makes sense.