Repository: vllm-project/vllm
Pull request: Allow users to pass arbitrary JSON keys from CLI
Closes #17640
Supersede #17842

Allows users to pass individual JSON keys via `FlexibleArgumentParser` in the format `--json-arg.key1.key2 value`

i.e. `--hf-overrides.key1 val1 --hf-overrides.key2.key3 val2` will result in the following args:

```console
INFO 05-15 17:09:16 [cli_args.py:297] non-default args: {'enforce_eager': True, 'hf_overrides': {'key1': 'val1', 'key2': {'key3': 'val2'}}, 'gpu_memory_utilization': 0.45}
```

Problem statement:
[Feature]: provide a way to configure rope-scaling that isn't inline JSON
### 🚀 The feature, motivation and pitch

Having to use literal JSON strings as a command argument is breaking a lot automation and scripts out there. I propose it's addressed so that all configuration can be done via simple key-value pairs.

### Alternatives

Instead of configuring it as follows:
```
vllm server [...]   --rope-scaling  '{"rope_type":"yarn","factor":4.0,"original_max_position_embeddings":32768}' 
```

I suggest something like:
```
vllm server [...]   --rope-scaling "1" \
--rope-type "yarn" \
--rope-factor "4.0" \
--original-max-position-embeddings "32768"

Hints from issue discussion:
@DarkLight1337  What do you think, If you agree, I can make changes.
I think this isn't necessary, you can simply pass a YAML config file to `vllm serve`.
@DarkLight1337 passing a yaml to `vllm serve` isn't a feasible solution for many situations. Most environments we work with can only be configured via ENV vars and command line arguments.
Could you elaborate on what is the issue with sending JSON in the command line arguments? Since different rope methods have different parameters, I worry that supporting such an interface becomes less maintainable over time.

cc @hmellor 
@DarkLight1337 
* Most automation instrumentation out there has been built on the "contract" that command line parameters are primitives (strings or numbers). Many tools that do the remote provisioning do URLencoding or similar transport-safe encoding before sending into a container manifest or some form of container provisioning. You cannot make the assumption the user has access to the command line and can simply paste the literal json blob like one can do in one's local computer or via ssh.
* using a yaml or json config for `vllm serve` is impracticable. If you manage dozens (or hundreds) of different inference backends, it's not reasonable to create and having to maintain dozens or hundreds of docker containers because each backend/model has its own params and because at deploy time the provider may not allow downloading or uploading of arbitrary files.
😬 

https://github.com/vllm-project/vllm/blob/f62cad6431e2bce91c033c74e20835c8e0c9b288/vllm/config.py#L453-L468
Since these are parameters from the Hugging Face `config.json` would it be possible for you to change their values at the source rather than patching at the end in vLLM?
@hmellor that would only work for models we control/own. But wouldn't work for ~dozens of base models we host/use.
I am more open to supporting this via nested namespaces (like in `jsonargparse`). Using your example, we could have:

```
vllm serve [...] \
    --hf-overrides.rope_scaling.rope_type yarn \
    --hf-overrides.rope_scaling.factor 4.0 \
    --hf-overrides.rope_scaling.original_max_position_embeddings 32768
```

That way we don't have to worry about defining the inner configs at the top level.
@DarkLight1337 that's a good compromise. Would be greatly helpful.

Commit message: Add comments

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>

Patch to review:
diff --git a/tests/test_utils.py b/tests/test_utils.py
index ea7db0a79c86..d760cdceb8ca 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -3,6 +3,7 @@
 
 import asyncio
 import hashlib
+import json
 import pickle
 import socket
 from collections.abc import AsyncIterator
@@ -138,6 +139,7 @@ def parser():
     parser.add_argument('--model-name')
     parser.add_argument('--batch-size', type=int)
     parser.add_argument('--enable-feature', action='store_true')
+    parser.add_argument('--hf-overrides', type=json.loads)
     return parser
 
 
@@ -251,6 +253,17 @@ def test_no_model_tag(parser_with_config, cli_config_file):
         parser_with_config.parse_args(['serve', '--config', cli_config_file])
 
 
+def test_dict_args(parser):
+    args = ["--hf-overrides.key1", "val1", "--hf-overrides.key2.key3", "val2"]
+    parsed_args = parser.parse_args(args)
+    assert parsed_args.hf_overrides == {
+        "key1": "val1",
+        "key2": {
+            "key3": "val2"
+        }
+    }
+
+
 # yapf: enable
 @pytest.mark.parametrize(
     "callable,kw_name,requires_kw_only,allow_var_kwargs,is_supported",
diff --git a/vllm/utils.py b/vllm/utils.py
index edfbb8c9481e..ab3ffe81d0ea 100644
--- a/vllm/utils.py
+++ b/vllm/utils.py
@@ -15,6 +15,7 @@
 import importlib.util
 import inspect
 import ipaddress
+import json
 import multiprocessing
 import os
 import pickle
@@ -1419,6 +1420,35 @@ def parse_args(  # type: ignore[override]
             else:
                 processed_args.append(arg)
 
+        def create_nested_dict(keys: list[str], value: str):
+            """Creates a nested dictionary from a list of keys and a value.
+
+            For example, `keys = ["a", "b", "c"]` and `value = 1` will create:
+            `{"a": {"b": {"c": 1}}}`
+            """
+            nested_dict: Any = value
+            for key in reversed(keys):
+                nested_dict = {key: nested_dict}
+            return nested_dict
+
+        dict_args: dict[str, dict] = defaultdict(dict)
+        # Loop in reverse because we are modifying the list
+        for i, processed_arg in reversed(list(enumerate(processed_args))):
+            if processed_arg.startswith("--") and "." in processed_arg:
+                if "=" in processed_arg:
+                    processed_arg, value = processed_arg.split("=", 1)
+                else:
+                    value = processed_args[i + 1]
+                    del processed_args[i + 1]
+                key, *keys = processed_arg.split(".")
+                # Merge all values with the same key into a single dict
+                dict_args[key].update(create_nested_dict(keys, value))
+                del processed_args[i]
+        # Add the dict args back as if they were originally passed as JSON
+        for dict_arg, dict_value in dict_args.items():
+            processed_args.append(dict_arg)
+            processed_args.append(json.dumps(dict_value))
+
         return super().parse_args(processed_args, namespace)
 
     def check_port(self, value):


Code review comments:
diff_hunk: @@ -1419,6 +1420,35 @@ def parse_args(  # type: ignore[override]
             else:
                 processed_args.append(arg)
 
+        def create_nested_dict(keys: list[str], value: str):
+            """Creates a nested dictionary from a list of keys and a value.
+
+            For example, `keys = ["a", "b", "c"]` and `value = 1` will create:
+            `{"a": {"b": {"c": 1}}}`
+            """
+            nested_dict: Any = value
+            for key in reversed(keys):
+                nested_dict = {key: nested_dict}
+            return nested_dict
+
+        dict_args: dict[str, dict] = defaultdict(dict)
+        # Loop in reverse because we are modifying the list
+        for i, processed_arg in reversed(list(enumerate(processed_args))):
+            if processed_arg.startswith("--") and "." in processed_arg:
+                if "=" in processed_arg:
+                    processed_arg, value = processed_arg.split("=", 1)
+                else:
+                    value = processed_args[i + 1]
+                    del processed_args[i + 1]
+                key, *keys = processed_arg.split(".")
+                # Merge all values with the same key into a single dict
+                dict_args[key].update(create_nested_dict(keys, value))
path: vllm/utils.py
text: @user1:
I think this would fail in a case like

```
--hf-overrides.key1.key2 a --hf-overrides.key1.key3 b
```

Because `{key2: a}` is replaced with `{key3: b}` for the key `key1` instead of being merged together.

@author:
Good point, I'll change this to recursively update