Closed tadejsv closed 1 year ago
Hi! Thank you for your contribution! Please re-check all issue template checklists - unfilled issues would be closed automatically. And do not forget to join our slack for collaboration.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
đ Feature Request
I propose to separate concerns between Accelerate and Catalyst: Catalyst should not try to spawn/launch anything - this should all be taken care of by
accelerate launch
.This would also remove
ddp
related settings fromRunner
s and remove all but the cpu and gpu engine [I noticed that a similar change was implemented in Animus].Motivation
Currently,
accelerate launch
and Catalyst do not play nice by default - if multiple GPUs are detected, Catalyst will try to do multiple process launching itself (this can be circumvented by directly specifying gpu engine).I think it is redundant to keep this duplicated functionality, and imo it would make more sense to offload it to Accelerate, as it is very actively developed.
Additional context
I guess the current design originated from a time when Accelerate was less powerful/organized than it is now, so now would be a good time to update it :)
This would be a breaking change for some users - i.e. those that used
ddp
settings from Runners or relied on the default behavior or launching multiple processes for when multiple GPUs are detected.For
ddp
setting, we could raise an error or a warning if it is provided, explaining to the user that the new way is to launch the script usingaccelerate launch
.