Repository: huggingface/accelerate
Pull request: raise error for duplicate accelerate config  values when using `deepspeed_config_file`
### What dos this PR do?
1. Fixes: #936 

Example:
1. accelerate config manually tweaked to have both `deepspeed_config_file` and other ds config entries that are available in the json config file:
```yaml
command_file: null
commands: null
compute_environment: LOCAL_MACHINE
deepspeed_config:
  gradient_accumulation_steps: 1
  gradient_clipping: 1.0
  offload_optimizer_device: 'cpu'
  offload_param_device: 'cpu'
  zero3_init_flag: true
  zero3_save_16bit_model: true
  zero_stage: 3
  deepspeed_config_file: 'ds_config.json'
distributed_type: DEEPSPEED
downcast_bf16: 'no'
dynamo_backend: 'NO'
fsdp_config: {}
gpu_ids: null
machine_rank: 0
main_process_ip: null
main_process_port: null
main_training_function: main
megatron_lm_config: {}
mixed_precision: 'bf16'
num_machines: 1
num_processes: 2
rdzv_backend: static
same_network: true
tpu_name: null
tpu_zone: null
use_cpu: false
```

2. ds_config.json:
```json
{
    "fp16": {
        "enabled": true
    },
    "zero_optimization": {
        "stage": 3,
        "stage3_gather_16bit_weights_on_model_save": false,
        "offload_optimizer": {
            "device": "none"
        },
        "offload_param": {
            "device": "none"
        }
    },
    "gradient_clipping": 1.0,
    "train_batch_size": "auto",
    "train_micro_batch_size_per_gpu": "auto",
    "gradient_accumulation_steps": 10,
    "steps_per_print": 2000000
}
```

3. Code:
```python
from accelerate import Accelerator

def main():
    accelerator = Accelerator()

if __name__ == "__main__":
    main()
```

4. output:
```bash
ValueError: When using `deepspeed_config_file`, the following accelerate config variables will be 
ignored: ['gradient_accumulation_steps', 'gradient_clipping', 'zero_stage', 
'offload_optimizer_device', 'offload_param_device', 'zero3_save_16bit_model', 'mixed_precision'].
Please specify them appropriately in the DeepSpeed config file.
If you are using accelerate config file, set `mixed_precision=no` and remove others config variables
mentioned in the above specified list; else don't specify these config variables in `accelerate 
launch` command. 
The easiest method is to create new config following the questionnaire via  `accelerate config`.
It will only ask for the necessary config variables when using `deepspeed_config_file`.
```

Problem statement:
[feature] assert on ambiguity config when using deepspeed
OK, so please consider this setup:

```
[...]
deepspeed_config:
  deepspeed_multinode_launcher: standard
  deepspeed_config_file: ds_config.json
  gradient_accumulation_steps: 2
  zero3_init_flag: true
  zero_stage: 2
  zero3_save_16bit_model: true
distributed_type: DEEPSPEED
[...]
```
and `ds_config.json`:
```
{
    "fp16": {
        "enabled": true
    },
    "zero_optimization": {
        "stage": 3,
        "stage3_gather_16bit_weights_on_model_save": false,
        "offload_optimizer": {
            "device": "none"
        },
        "offload_param": {
            "device": "none"
        }
    },
    "gradient_clipping": 1.0,
    "train_batch_size": "auto",
    "train_micro_batch_size_per_gpu": "auto",
    "gradient_accumulation_steps": 10,
    "steps_per_print": 2000000
}
```

as you can see we have a potential difficult situation here as there are the same configs defined in multiple places. e.g. `zero3_save_16bit_model`, `gradient_accumulation_steps`, `zero_stage`, etc. with values that don't match! And the user isn't the wiser which of them prevail.

Here is my proposal: 

If `deepspeed_config_file` is provided, Accelerate should assert on any other configs that will collide with it. And unless I'm missing some details, I think the only valid (unique) entries of `deepspeed_config` that aren't in the DS config file are:
```
deepspeed_config:
  deepspeed_multinode_launcher: standard
  deepspeed_config_file: ds_config.json
  zero3_init_flag: true
distributed_type: DEEPSPEED
```

and any other deepspeed_config entries create ambiguity and should be asserted on.

And of course if `deepspeed_config_file` is not provided, then of course all those other options are a must.

OK, but then we have the `auto` feature which is useful when things are set elsewhere. So the categorical proposition I made above is probably incorrect. Yet, as you can see from the test case I presented - there is a massive mismatch between the 2 config files and accelerate doesn't assert on that at all. This situation makes debugging extremely painful since I can never be sure if I changed the right config and I'm testing the right combination. And yes, I can look at the logs, but this is not conducive for easy debug.

for context: I spent several days debugging a bad memory leak in m4, triggered by `zero.Init` and I had to switch multiple flags on/off while doing the debug many times again and again.

Thank you!

Hints from issue discussion:
cc @pacman100 
Hello @stas00 , thank you for providing a detailed issue. 

One important thing to note from the current design standpoint is that when using the `accelerate config` command to fill out the questionnaire if the user chooses to provide `deepspeed_config_file`, other options aren't asked at all and therefore don't lead to ambiguity scenario. Below is the questionnaire flow along with the final yaml config file when using `deepspeed_config_file` entry:

![Screenshot 2022-12-22 at 1 46 59 PM](https://user-images.githubusercontent.com/13534540/209089022-e20e42d7-043e-4781-9ca3-e7488bb79714.png)

![Screenshot 2022-12-22 at 1 47 31 PM](https://user-images.githubusercontent.com/13534540/209089116-23d4dcfb-4cb5-4613-a578-3cea274badba.png)

Therefore, when the user is willing to provide the `deepspeed_config_file`, only `zero3_init_flag` entry is asked and others are ignored as they will be part of the json config file that the user provides. This avoids any ambiguous scenario. A bit weird that the config file you shared has the location `deepspeed_config_file` along with other entries.

Thank you for explaining your design, @pacman100 

>  A bit weird that the config file you shared has the location deepspeed_config_file along with other entries.

It didn't. That's not how we configure accelerate at m4 - we have various config templates that we manually tweak. I use accelerate docs/code to figure out which other config params we can use. 

I think relying on output of a certain program to never change and requiring a user to re-run an interview for every tiny change in setup is not the most user-friendly solution.

If the config's correctness depends on a user not tampering with the config file, then it shouldn't be a text file, but some binary format, but of course that would be madness.

Is it very difficult to validate the incoming configuration and assert on any conflicts? e.g. as I identified the low-hanging fruit would be to refuse any but `zero3_init_flag` config entries when `deepspeed_config_file` is used (as it's the only config that DS config doesn't already have). That would solve any ambiguity at once.
Hello @stas00, please let us know if the above PR addresses the requirements of this issue

Thank you, @pacman100 - I will review it today.

I realized we have the same issue with the `mixed_precision` config - Accelerate probably should assert when it's configured and ds_config is used, no? as this is another source of ambiguity 
> realized we have the same issue with the `mixed_precision` config - Accelerate probably should assert when it's configured and ds_config is used, no? as this is another source of ambiguity

Done

Commit message: address comment

Patch to review:
diff --git a/src/accelerate/commands/launch.py b/src/accelerate/commands/launch.py
index 91d4427ac5a..eb641c4c650 100644
--- a/src/accelerate/commands/launch.py
+++ b/src/accelerate/commands/launch.py
@@ -301,45 +301,50 @@ def launch_command_parser(subparsers=None):
         "--zero_stage",
         default=None,
         type=int,
-        help="DeepSpeed's ZeRO optimization stage (useful only when `use_deepspeed` flag is passed).",
+        help="DeepSpeed's ZeRO optimization stage (useful only when `use_deepspeed` flag is passed). "
+        "If unspecified, will default to `2`.",
     )
     deepspeed_args.add_argument(
         "--offload_optimizer_device",
         default=None,
         type=str,
-        help="Decides where (none|cpu|nvme) to offload optimizer states (useful only when `use_deepspeed` flag is passed).",
+        help="Decides where (none|cpu|nvme) to offload optimizer states (useful only when `use_deepspeed` flag is passed). "
+        "If unspecified, will default to 'none'.",
     )
     deepspeed_args.add_argument(
         "--offload_param_device",
         default=None,
         type=str,
-        help="Decides where (none|cpu|nvme) to offload parameters (useful only when `use_deepspeed` flag is passed).",
+        help="Decides where (none|cpu|nvme) to offload parameters (useful only when `use_deepspeed` flag is passed). "
+        "If unspecified, will default to 'none'.",
     )
     deepspeed_args.add_argument(
         "--gradient_accumulation_steps",
         default=None,
         type=int,
-        help="No of gradient_accumulation_steps used in your training script (useful only when `use_deepspeed` flag is passed).",
+        help="No of gradient_accumulation_steps used in your training script (useful only when `use_deepspeed` flag is passed). "
+        "If unspecified, will default to `1`.",
     )
     deepspeed_args.add_argument(
         "--gradient_clipping",
         default=None,
         type=float,
-        help="gradient clipping value used in your training script (useful only when `use_deepspeed` flag is passed).",
+        help="gradient clipping value used in your training script (useful only when `use_deepspeed` flag is passed). "
+        "If unspecified, will default to `1.0`.",
     )
     deepspeed_args.add_argument(
         "--zero3_init_flag",
-        default="true",
+        default=None,
         type=str,
         help="Decides Whether (true|false) to enable `deepspeed.zero.Init` for constructing massive models. "
-        "Only applicable with DeepSpeed ZeRO Stage-3.",
+        "Only applicable with DeepSpeed ZeRO Stage-3. If unspecified, will default to `true`.",
     )
     deepspeed_args.add_argument(
         "--zero3_save_16bit_model",
-        default="false",
+        default=None,
         type=str,
         help="Decides Whether (true|false) to save 16-bit model weights when using ZeRO Stage-3. "
-        "Only applicable with DeepSpeed ZeRO Stage-3.",
+        "Only applicable with DeepSpeed ZeRO Stage-3. If unspecified, will default to `false`.",
     )
     deepspeed_args.add_argument(
         "--deepspeed_hostfile",
@@ -363,7 +368,7 @@ def launch_command_parser(subparsers=None):
         "--deepspeed_multinode_launcher",
         default=None,
         type=str,
-        help="DeepSpeed multi-node launcher to use.",
+        help="DeepSpeed multi-node launcher to use. If unspecified, will default to `pdsh`.",
     )
 
     # fsdp arguments
@@ -718,13 +723,20 @@ def deepspeed_launcher(args):
     current_env["PYTHONPATH"] = env_var_path_add("PYTHONPATH", os.path.abspath("."))
     current_env["ACCELERATE_MIXED_PRECISION"] = str(mixed_precision)
     current_env["ACCELERATE_USE_DEEPSPEED"] = "true"
-    current_env["ACCELERATE_DEEPSPEED_ZERO_STAGE"] = str(args.zero_stage)
-    current_env["ACCELERATE_GRADIENT_ACCUMULATION_STEPS"] = str(args.gradient_accumulation_steps)
-    current_env["ACCELERATE_GRADIENT_CLIPPING"] = str(args.gradient_clipping).lower()
-    current_env["ACCELERATE_DEEPSPEED_OFFLOAD_OPTIMIZER_DEVICE"] = str(args.offload_optimizer_device).lower()
-    current_env["ACCELERATE_DEEPSPEED_OFFLOAD_PARAM_DEVICE"] = str(args.offload_param_device).lower()
-    current_env["ACCELERATE_DEEPSPEED_ZERO3_INIT"] = str(args.zero3_init_flag).lower()
-    current_env["ACCELERATE_DEEPSPEED_ZERO3_SAVE_16BIT_MODEL"] = str(args.zero3_save_16bit_model).lower()
+    if args.zero_stage is not None:
+        current_env["ACCELERATE_DEEPSPEED_ZERO_STAGE"] = str(args.zero_stage)
+    if args.gradient_accumulation_steps is not None:
+        current_env["ACCELERATE_GRADIENT_ACCUMULATION_STEPS"] = str(args.gradient_accumulation_steps)
+    if args.gradient_clipping is not None:
+        current_env["ACCELERATE_GRADIENT_CLIPPING"] = str(args.gradient_clipping).lower()
+    if args.offload_optimizer_device is not None:
+        current_env["ACCELERATE_DEEPSPEED_OFFLOAD_OPTIMIZER_DEVICE"] = str(args.offload_optimizer_device).lower()
+    if args.offload_param_device is not None:
+        current_env["ACCELERATE_DEEPSPEED_OFFLOAD_PARAM_DEVICE"] = str(args.offload_param_device).lower()
+    if args.zero3_init_flag is not None:
+        current_env["ACCELERATE_DEEPSPEED_ZERO3_INIT"] = str(args.zero3_init_flag).lower()
+    if args.zero3_save_16bit_model is not None:
+        current_env["ACCELERATE_DEEPSPEED_ZERO3_SAVE_16BIT_MODEL"] = str(args.zero3_save_16bit_model).lower()
     if args.deepspeed_config_file is not None:
         current_env["ACCELERATE_DEEPSPEED_CONFIG_FILE"] = str(args.deepspeed_config_file)
 
diff --git a/src/accelerate/utils/dataclasses.py b/src/accelerate/utils/dataclasses.py
index 5a87d786017..2fea1ab9efa 100644
--- a/src/accelerate/utils/dataclasses.py
+++ b/src/accelerate/utils/dataclasses.py
@@ -409,6 +409,8 @@ def __post_init__(self):
                 raise ValueError("gradient_accumulation_steps cannot be set to 'auto' in the DeepSpeed config.")
             if "zero_optimization" not in self.hf_ds_config.config:
                 raise ValueError("Please specify the ZeRO optimization config in the DeepSpeed config.")
+
+            self._deepspeed_config_checks()
         else:
             if self.gradient_accumulation_steps is None:
                 self.gradient_accumulation_steps = int(os.environ.get("ACCELERATE_GRADIENT_ACCUMULATION_STEPS", 1))
@@ -453,7 +455,7 @@ def __post_init__(self):
         self.deepspeed_config = self.hf_ds_config.config
         self.deepspeed_config["steps_per_print"] = float("inf")  # this will stop deepspeed from logging @ stdout
         if self.zero3_init_flag is None:
-            self.zero3_init_flag = os.environ.get("ACCELERATE_DEEPSPEED_ZERO3_INIT", "false") == "true"
+            self.zero3_init_flag = os.environ.get("ACCELERATE_DEEPSPEED_ZERO3_INIT", "true") == "true"
         if self.zero3_init_flag and not self.hf_ds_config.is_zero3():
             warnings.warn("DeepSpeed Zero3 Init flag is only applicable for ZeRO Stage 3. Setting it to False.")
             self.zero3_init_flag = False
@@ -549,6 +551,40 @@ def zero3_init_context_manager(self, enable=False):
             self.dschf = None
             self.set_deepspeed_weakref()
 
+    def _deepspeed_config_checks(self):
+        env_variable_names_to_ignore = [
+            "ACCELERATE_GRADIENT_ACCUMULATION_STEPS",
+            "ACCELERATE_GRADIENT_CLIPPING",
+            "ACCELERATE_DEEPSPEED_ZERO_STAGE",
+            "ACCELERATE_DEEPSPEED_OFFLOAD_OPTIMIZER_DEVICE",
+            "ACCELERATE_DEEPSPEED_OFFLOAD_PARAM_DEVICE",
+            "ACCELERATE_DEEPSPEED_ZERO3_SAVE_16BIT_MODEL",
+            "ACCELERATE_MIXED_PRECISION",
+        ]
+        duplicate_values_flag = False
+        for name in env_variable_names_to_ignore:
+            if name != "ACCELERATE_MIXED_PRECISION" and os.environ.get(name, None) is not None:
+                duplicate_values_flag = True
+            elif name == "ACCELERATE_MIXED_PRECISION" and os.environ.get(name, "no") != "no":
+                duplicate_values_flag = True
+            if duplicate_values_flag:
+                break
+
+        if duplicate_values_flag:
+            env_variable_names_to_ignore = [
+                name.replace("ACCELERATE_", "").replace("DEEPSPEED_", "").lower()
+                for name in env_variable_names_to_ignore
+            ]
+            raise ValueError(
+                f"When using `deepspeed_config_file`, the following accelerate config variables will be ignored: {env_variable_names_to_ignore}.\n"
+                "Please specify them appropriately in the DeepSpeed config file.\n"
+                "If you are using an accelerate config file, set `mixed_precision=no` "
+                "and remove others config variables mentioned in the above specified list; "
+                "and make sure to not specify these config variables in `accelerate launch` command. \n"
+                "The easiest method is to create new config following the questionnaire via  `accelerate config`.\n"
+                "It will only ask for the necessary config variables when using `deepspeed_config_file`."
+            )
+
 
 @dataclass
 class FullyShardedDataParallelPlugin:


Code review comments:
diff_hunk: @@ -549,6 +551,40 @@ def zero3_init_context_manager(self, enable=False):
             self.dschf = None
             self.set_deepspeed_weakref()
 
+    def _deepspeed_config_checks(self):
+        env_variable_names_to_ignore = [
+            "ACCELERATE_GRADIENT_ACCUMULATION_STEPS",
+            "ACCELERATE_GRADIENT_CLIPPING",
+            "ACCELERATE_DEEPSPEED_ZERO_STAGE",
+            "ACCELERATE_DEEPSPEED_OFFLOAD_OPTIMIZER_DEVICE",
+            "ACCELERATE_DEEPSPEED_OFFLOAD_PARAM_DEVICE",
+            "ACCELERATE_DEEPSPEED_ZERO3_SAVE_16BIT_MODEL",
+            "ACCELERATE_MIXED_PRECISION",
+        ]
+        duplicate_values_flag = False
+        for name in env_variable_names_to_ignore:
+            if name != "ACCELERATE_MIXED_PRECISION" and os.environ.get(name, None) is not None:
+                duplicate_values_flag = True
+            elif name == "ACCELERATE_MIXED_PRECISION" and os.environ.get(name, "no") != "no":
+                duplicate_values_flag = True
+            if duplicate_values_flag:
+                break
+
+        if duplicate_values_flag:
+            env_variable_names_to_ignore = [
+                name.replace("ACCELERATE_", "").replace("DEEPSPEED_", "").lower()
+                for name in env_variable_names_to_ignore
+            ]
+            raise ValueError(
+                f"When using `deepspeed_config_file`, the following accelerate config variables will be ignored: {env_variable_names_to_ignore}.\n"
+                "Please specify them appropriately in the DeepSpeed config file.\n"
+                "If you are using an accelerate config file, set `mixed_precision=no` "
path: src/accelerate/utils/dataclasses.py
text: @user1:
Did I understand it right that here you suggest to the user to set `mixed_precision=no` even if they use mixed precision in ds_config.json? If so this now is worse than before, as now you're proposing to force 2 opposite values in the same config entry. As in `mixed_precision=no` and `fp16 { enabled: true}`.

Unless I'm misunderstanding the wording that is. 

I think it should be either `mixed_precision=xyz` or `ds_config.json` (with fp16 or bf16 blocks or no block at all as fp16 is the default), but not both.

@author:
Done. I've simplified the error message wherein user can just remove all the ambiguous entries from  accelerate config file and not specify them in launch command

@user1:
to focus we are dealing with `mixed_precision` duplicity here:

I've just tried:

```
deepspeed_config:
  deepspeed_multinode_launcher: standard
  deepspeed_config_file: ./configs/vopt-large-z3/ds_config.json
  zero3_init_flag: true
distributed_type: DEEPSPEED
mixed_precision: 'no'
```

```
{
    "fp16": {
        "enabled": true,
        [...]
```
and it doesn't assert.

with the latest commit of `dd2d57b6a1`



@user1:
(incidentally `mixed_precision: fp16|bf16|'no'` is odd - why do different values don't follow the same style - i.e 'no' in quotes but not the other values)

@author:
Hello @user2, this case can't be helped as 'no' is the default value and if one doesn't specify the entry in accelerate config file, it will get default value of 'no' and thereby we have no way to check if user has given the default in config file or was it set to default when reading the config file. It would mean a lot of code rewriting as `mixed_precision` is used in various other non DeepSpeed parts.

The default value of `mixed_precision` would be overriden by that in `deepspeed_config_file`. Now, if you answer the questionnaire via `accelerate config`, the entry of `mixed_precision` won't be there when using `deepspeed_config_file`. 


@user3, what are your thoughts around this. 

@user1:
understood! in which case I think this should work:

1. if the `mixed_precision` config isn't in the accelerate config file all is good
2. if the `mixed_precision` config is in the accelerate config file and ds_config file is used, the value of the former must match the value of the latter or assert.