Repository: skypilot-org/skypilot
Pull request: improve CLI config flag's parsing
<!-- Describe the changes in this PR -->
Closes https://github.com/skypilot-org/skypilot/issues/5296

The CLI config flag currently accepts a comma separated list of config overrides. For example:
`--config kubernetes.provision_timeout=600,kubernetes.pod_config.spec.priorityClassName=high-priority`
is a valid CLI flag.

The CLI parsing logic currently simply splits the input string by commas and feed the result into a dotlist parser. While this logic works for example above, the logic fails at parsing config overrides such as:
`--config kubernetes.allowed_contexts=[context1,context2]`
or
`--config gcp.labels.mylabel="one,two,three"`
because CLI splits the input by the commas within the value designation.

I've originally attempted to solve it by having the parsing logic account for curly braces, square braces and quotes - but the number of edge cases to deal with kept increasing as I iterated on this logic.

I think the simplest way to provide robust parsing here is to let users pass in each config overrides in a separate command.

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [ ] Code formatting: install pre-commit (auto-check on commit) or `bash format.sh`
- [ ] Any manual or new tests for this PR (please specify below)
- [ ] All smoke tests: `/smoke-test` (CI) or `pytest tests/test_smoke.py` (local)
- [ ] Relevant individual tests: `/smoke-test -k test_name` (CI) or `pytest tests/test_smoke.py::test_name` (local)
- [ ] Backward compatibility: `/quicktest-core` (CI) or `pytest tests/smoke_tests/test_backward_compat.py` (local)

<!-- CI commands (/-prefixed) can only be triggered by repo members -->


Problem statement:
[config] CLI "--config" flag does not properly parse commas passed as values
The CLI config flag accepts a comma separated list of config overrides. For example:
`--config kubernetes.provision_timeout=600,kubernetes.pod_config.spec.priorityClassName=high-priority`
is a valid CLI flag.

The CLI parsing logic currently simply splits the input string by commas and feed the result into a dotlist parser. While this logic works for example above, the logic fails at parsing config overrides such as:
`--config kubernetes.allowed_contexts=[context1,context2]`
or
`--config gcp.labels.mylabel="one,two,three"`
because CLI splits the input by the commas within the value designation.

Hints from issue discussion:

Commit message: improve CLI config flag's parsing

Patch to review:
diff --git a/docs/source/reference/config-sources.rst b/docs/source/reference/config-sources.rst
index d09445146d4..8a213602686 100644
--- a/docs/source/reference/config-sources.rst
+++ b/docs/source/reference/config-sources.rst
@@ -138,7 +138,8 @@ CLI flag
 
 You can pass configuration arguments to the CLI using the ``--config`` flag.
 
-The ``--config`` flag can either be a path to a config YAML file, or a dotlist of key-value pairs. Only one ``--config`` flag can be provided.
+The ``--config`` flag can either be a path to a config YAML file, or a dotlist of key-value pairs.
+If passing in a config file, only one ``--config`` flag can be provided.
 
 Example:
 
@@ -147,8 +148,8 @@ Example:
   # pass a config file
   sky launch --config my_config.yaml ...
   # pass individual config options
-  sky launch --config 'kubernetes.provision_timeout=600,kubernetes.pod_config.spec.priorityClassName=high-priority' ...
-  sky launch --config 'kubernetes.custom_metadata.annotations.myannotation1=myvalue1,kubernetes.custom_metadata.annotations.myannotation2=myvalue2' ...
+  sky launch --config 'kubernetes.provision_timeout=600' --config 'kubernetes.pod_config.spec.priorityClassName=high-priority' ...
+  sky launch --config 'kubernetes.custom_metadata.annotations.myannotation1=myvalue1' --config 'kubernetes.custom_metadata.annotations.myannotation2=myvalue2' ...
 
 
 .. _config-overrides:
diff --git a/sky/cli.py b/sky/cli.py
index 5d7b7854348..cc873d184fd 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -302,13 +302,9 @@ def preprocess_config_options(ctx, param, value):
         try:
             if len(value) == 0:
                 return None
-            elif len(value) > 1:
-                raise ValueError('argument specified multiple times. '
-                                 'To specify multiple configs, use '
-                                 '--config nested.key1=val1,another.key2=val2')
             else:
                 # Apply the config overrides to the skypilot config.
-                return skypilot_config.apply_cli_config(value[0])
+                return skypilot_config.apply_cli_config(value)
         except ValueError as e:
             raise click.BadParameter(f'{str(e)}') from e
 
diff --git a/sky/skypilot_config.py b/sky/skypilot_config.py
index cfe1854a66e..0a5255518ea 100644
--- a/sky/skypilot_config.py
+++ b/sky/skypilot_config.py
@@ -464,7 +464,7 @@ def override_skypilot_config(
         _config_overridden = False
 
 
-def _compose_cli_config(cli_config: Optional[str],) -> config_utils.Config:
+def _compose_cli_config(cli_config: Optional[List[str]]) -> config_utils.Config:
     """Composes the skypilot CLI config.
     CLI config can either be:
     - A path to a config file
@@ -475,18 +475,16 @@ def _compose_cli_config(cli_config: Optional[str],) -> config_utils.Config:
         return config_utils.Config()
 
     config_source = 'CLI'
-    maybe_config_path = os.path.expanduser(cli_config)
     try:
-        if os.path.isfile(maybe_config_path):
+        maybe_config_path = os.path.expanduser(cli_config[0])
+        if len(cli_config) == 1 and os.path.isfile(maybe_config_path):
             config_source = maybe_config_path
             # cli_config is a path to a config file
             parsed_config = OmegaConf.to_object(
                 OmegaConf.load(maybe_config_path))
         else:  # cli_config is a comma-separated list of key-value pairs
-            variables: List[str] = []
-            variables = cli_config.split(',')
             parsed_config = OmegaConf.to_object(
-                OmegaConf.from_dotlist(variables))
+                OmegaConf.from_dotlist(cli_config))
         _validate_config(parsed_config, config_source)
     except ValueError as e:
         raise ValueError(f'Invalid config override: {cli_config}. '
@@ -497,7 +495,7 @@ def _compose_cli_config(cli_config: Optional[str],) -> config_utils.Config:
     return parsed_config
 
 
-def apply_cli_config(cli_config: Optional[str]) -> Dict[str, Any]:
+def apply_cli_config(cli_config: Optional[List[str]]) -> Dict[str, Any]:
     """Applies the CLI provided config.
     SAFETY:
     This function directly modifies the global _dict variable.


Code review comments:
diff_hunk: @@ -475,18 +475,16 @@ def _compose_cli_config(cli_config: Optional[str],) -> config_utils.Config:
         return config_utils.Config()
 
     config_source = 'CLI'
-    maybe_config_path = os.path.expanduser(cli_config)
     try:
-        if os.path.isfile(maybe_config_path):
+        maybe_config_path = os.path.expanduser(cli_config[0])
+        if len(cli_config) == 1 and os.path.isfile(maybe_config_path):
path: sky/skypilot_config.py
text: @user1:
```suggestion
        if os.path.isfile(maybe_config_path):
            if len(cli_config) != 1:
                raise ValueError('Cannot use multiple --config flags with a config file.')
```
Just to make sure the error is more clear in this case.