Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] Fix load config when using bools #9533

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions tests/data/test_config.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
port: 12312
served_model_name: mymodel
tensor_parallel_size: 2
trust_remote_code: true
multi_step_stream_outputs: false
6 changes: 5 additions & 1 deletion tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import pytest

from vllm.utils import (FlexibleArgumentParser, deprecate_kwargs,
from vllm.utils import (FlexibleArgumentParser, StoreBoolean, deprecate_kwargs,
get_open_port, merge_async_iterators, supports_kw)

from .utils import error_on_warning
Expand Down Expand Up @@ -141,6 +141,8 @@ def parser_with_config():
parser.add_argument('--config', type=str)
parser.add_argument('--port', type=int)
parser.add_argument('--tensor-parallel-size', type=int)
parser.add_argument('--trust-remote-code', action='store_true')
parser.add_argument('--multi-step-stream-outputs', action=StoreBoolean)
return parser


Expand Down Expand Up @@ -214,6 +216,8 @@ def test_config_args(parser_with_config):
args = parser_with_config.parse_args(
['serve', 'mymodel', '--config', './data/test_config.yaml'])
assert args.tensor_parallel_size == 2
assert args.trust_remote_code
assert not args.multi_step_stream_outputs


def test_config_file(parser_with_config):
Expand Down
14 changes: 1 addition & 13 deletions vllm/engine/arg_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from vllm.transformers_utils.config import (
maybe_register_config_serialize_by_value)
from vllm.transformers_utils.utils import check_gguf_file
from vllm.utils import FlexibleArgumentParser
from vllm.utils import FlexibleArgumentParser, StoreBoolean

if TYPE_CHECKING:
from vllm.transformers_utils.tokenizer_group import BaseTokenizerGroup
Expand Down Expand Up @@ -1144,18 +1144,6 @@ def add_cli_args(parser: FlexibleArgumentParser,
return parser


class StoreBoolean(argparse.Action):

def __call__(self, parser, namespace, values, option_string=None):
if values.lower() == "true":
setattr(namespace, self.dest, True)
elif values.lower() == "false":
setattr(namespace, self.dest, False)
else:
raise ValueError(f"Invalid boolean value: {values}. "
"Expected 'true' or 'false'.")


# These functions are used by sphinx to build the documentation
def _engine_args_parser():
return EngineArgs.add_cli_args(FlexibleArgumentParser())
Expand Down
35 changes: 27 additions & 8 deletions vllm/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,18 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> None:
return wrapper


class StoreBoolean(argparse.Action):

def __call__(self, parser, namespace, values, option_string=None):
if values.lower() == "true":
setattr(namespace, self.dest, True)
elif values.lower() == "false":
setattr(namespace, self.dest, False)
else:
raise ValueError(f"Invalid boolean value: {values}. "
"Expected 'true' or 'false'.")


class FlexibleArgumentParser(argparse.ArgumentParser):
"""ArgumentParser that allows both underscore and dash in names."""

Expand All @@ -1163,7 +1175,7 @@ def parse_args(self, args=None, namespace=None):
args = sys.argv[1:]

if '--config' in args:
args = FlexibleArgumentParser._pull_args_from_config(args)
args = self._pull_args_from_config(args)

# Convert underscores to dashes and vice versa in argument names
processed_args = []
Expand All @@ -1181,8 +1193,7 @@ def parse_args(self, args=None, namespace=None):

return super().parse_args(processed_args, namespace)

@staticmethod
def _pull_args_from_config(args: List[str]) -> List[str]:
def _pull_args_from_config(self, args: List[str]) -> List[str]:
"""Method to pull arguments specified in the config file
into the command-line args variable.

Expand Down Expand Up @@ -1226,7 +1237,7 @@ def _pull_args_from_config(args: List[str]) -> List[str]:

file_path = args[index + 1]

config_args = FlexibleArgumentParser._load_config_file(file_path)
config_args = self._load_config_file(file_path)

# 0th index is for {serve,chat,complete}
# followed by model_tag (only for serve)
Expand All @@ -1247,8 +1258,7 @@ def _pull_args_from_config(args: List[str]) -> List[str]:

return args

@staticmethod
def _load_config_file(file_path: str) -> List[str]:
def _load_config_file(self, file_path: str) -> List[str]:
"""Loads a yaml file and returns the key value pairs as a
flattened list with argparse like pattern
```yaml
Expand Down Expand Up @@ -1282,9 +1292,18 @@ def _load_config_file(file_path: str) -> List[str]:
Make sure path is correct", file_path)
raise ex

store_boolean_arguments = [
action.dest for action in self._actions
if isinstance(action, StoreBoolean)
]

for key, value in config.items():
processed_args.append('--' + key)
processed_args.append(str(value))
if isinstance(value, bool) and key not in store_boolean_arguments:
if value:
processed_args.append('--' + key)
Comment on lines +1302 to +1303
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have some boolean arguments that aren't just store_true. I know you can specify --trust-remote-code=True on the CLI so maybe the issue could also be solved by the mismatch of capitalization on true vs True - what do you think?

Copy link
Author

@madt2709 madt2709 Oct 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I searched for store_false on this file: https://github.com/vllm-project/vllm/blob/main/vllm/engine/arg_utils.py and couldn't find any instances of it.

However, with a second look, there's two types of boolean flags:

  • action='store_true' eg --trust-remote-code
  • action=StoreBoolean eg --disable-logprobs-during-spec-decoding.

In the store_true case, --trust-remote-code=True doesn't work eg run vllm serve facebook/opt-125m --trust-remote-code=True and it errors with vllm serve: error: argument --trust-remote-code: ignored explicit argument 'True'.

In the StoreBoolean case, --disable-logprobs-during-spec-decoding=True does work.

So I think my code needs to add an extra check for these StoreBoolean arguments and explicitly set the value in that case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR to handle StoreBoolean values.

  • Moved StoreBoolean to vllm/utils.py to avoid circular imports
  • Updated the _load_config_from_file to not be static so it can check which arguments are in StoreBoolean case
  • Added a check for the StoreBoolean

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you were right about the difference there, but yes I was referring to the usage of StoreBoolean! Thanks for looking into this

else:
processed_args.append('--' + key)
processed_args.append(str(value))

return processed_args

Expand Down