-
Notifications
You must be signed in to change notification settings - Fork 23
feat: node param override #56
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
base: devel
Are you sure you want to change the base?
Changes from all commits
2006eae
7c854a4
495f172
bfede6c
5b1e63a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,7 @@ class BetterLaunch(metaclass=BetterLaunchMeta): | |
|
|
||
| _launchfile: str = None | ||
| _launch_func_args: dict[str, Any] = {} | ||
| _node_param_overrides: dict[str, dict[str, Any]] = {} | ||
|
|
||
| def __init__( | ||
| self, | ||
|
|
@@ -224,6 +225,9 @@ def hello(self) -> None: | |
| # We don't want to log this | ||
| print(msg) | ||
|
|
||
| def set_node_param_overrides(self, overrides: dict[str, dict[str, Any]]) -> None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A setter only member feels weird to me. Since it's intended to be modified and no other checks are done I think you can just make the member public. |
||
| BetterLaunch._node_param_overrides = overrides or {} | ||
|
|
||
| def spin(self, exit_with_last_node: bool = True) -> None: | ||
| """Join the BetterLaunch thread until it terminates. You do **not** need to call this if you're using the [launch_this][] wrapper or the TUI. | ||
|
|
||
|
|
@@ -1506,13 +1510,26 @@ def node( | |
| group = self.group_tip | ||
| namespace = group.assemble_namespace() | ||
|
|
||
| # Merge CLI overrides (CLI takes precedence) | ||
| resolved_params: dict[str, Any] | ||
| if isinstance(params, str): | ||
| resolved_params = self.load_params(configfile=params) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to add a |
||
| elif params is None: | ||
| resolved_params = {} | ||
| else: | ||
| resolved_params = params | ||
|
|
||
| cli_overrides = self._node_param_overrides.get(name, {}) | ||
| if cli_overrides: | ||
| resolved_params = {**resolved_params, **cli_overrides} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use |
||
|
|
||
| node = Node( | ||
| package, | ||
| executable, | ||
| name, | ||
| namespace, | ||
| remaps=remaps, | ||
| params=params, | ||
| params=resolved_params, | ||
| cmd_args=cmd_args, | ||
| env=env, | ||
| isolate_env=isolate_env, | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think this should be part of this PR |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| from typing import Any, Type, Iterable, Callable | ||
| import json | ||
| from dataclasses import dataclass | ||
| import click | ||
|
|
||
|
|
@@ -53,20 +54,46 @@ def get_click_bl_options(expose: bool = False) -> list[click.Option]: | |
| list[click.Option] | ||
| _description_ | ||
| """ | ||
|
|
||
| def update_value(ctx: click.Context, param: click.Parameter, value: Any): | ||
| key = param.name[3:].replace("-", "_") | ||
| _update_settings(**{key: value}) | ||
|
|
||
| if value is not None: | ||
| _update_settings(**{key: value}) | ||
|
|
||
| if key == "node_param_override" and value: | ||
| ctx.command.allow_extra_args = True | ||
| ctx.allow_extra_args = True | ||
|
|
||
| def _bool_param_type(value: str) -> bool: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. click already handles bool types well, don't think this is needed |
||
| """Convert string to boolean for Click options.""" | ||
| if isinstance(value, bool): | ||
| return value | ||
| val_lower = value.lower() | ||
| if val_lower in ("true", "1", "yes", "on"): | ||
| return True | ||
| if val_lower in ("false", "0", "no", "off"): | ||
| return False | ||
| raise click.BadParameter(f"Invalid boolean value: {value}") | ||
|
|
||
| # XXX always keep these synchronized with our Settings class | ||
| options = [ | ||
| click.Option( | ||
| ["--bl-ui"], | ||
| type=bool, | ||
| type=_bool_param_type, | ||
| default=None, | ||
| help="Enforce or prevent starting the TUI", | ||
| expose_value=expose, # not passed to our run method | ||
| callback=update_value, | ||
| ), | ||
| click.Option( | ||
| ["--bl-node-param-override"], | ||
| type=_bool_param_type, | ||
| default=None, | ||
| help="Allow overriding node parameters from the command line", | ||
| expose_value=expose, | ||
| callback=update_value, | ||
| ), | ||
| click.Option( | ||
| ["--bl-colormode"], | ||
| type=click.types.Choice([c.name for c in Colormode], case_sensitive=False), | ||
|
|
@@ -138,11 +165,88 @@ def get_click_launch_command( | |
| allow_kwargs: bool = False, | ||
| ) -> click.Command: | ||
| click_cmd = click.Command( | ||
| cmd_name, callback=launch_func, params=options, help=cmd_help | ||
| cmd_name, | ||
| callback=launch_func, | ||
| params=options, | ||
| help=cmd_help, | ||
| context_settings={"ignore_unknown_options": True}, | ||
| ) | ||
|
|
||
| if allow_kwargs: | ||
| click_cmd.allow_extra_args = True | ||
| click_cmd.ignore_unknown_options = True | ||
| click_cmd.allow_extra_args = allow_kwargs | ||
| click_cmd.ignore_unknown_options = True | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like having this on by default, is this needed for this PR? |
||
|
|
||
| return click_cmd | ||
|
|
||
|
|
||
| def args_to_dict(args: list[str]) -> dict[str, Any]: | ||
| """Convert a list of CLI arguments to a dictionary. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| args : list[str] | ||
| List of arguments, e.g. ["--foo", "bar", "--baz", "1.0"] | ||
|
|
||
| Returns | ||
| ------- | ||
| dict[str, Any] | ||
| Dictionary of arguments, e.g. {"foo": "bar", "baz": 1.0} | ||
|
|
||
| Raises | ||
| ------ | ||
| ValueError | ||
| If an argument does not start with "-" or if a value is missing. | ||
| """ | ||
| result = {} | ||
| it = iter(args) | ||
| for arg in it: | ||
| if not arg.startswith("-"): | ||
| raise ValueError(f"Argument '{arg}' does not start with '-'") | ||
|
|
||
| key = arg.lstrip("-") | ||
| try: | ||
| value = next(it) | ||
| except StopIteration: | ||
| raise ValueError(f"Missing value for argument '{arg}'") | ||
|
|
||
| try: | ||
| value = json.loads(value) | ||
| except (json.JSONDecodeError, TypeError): | ||
| pass | ||
|
|
||
| result[key] = value | ||
|
|
||
| return result | ||
|
|
||
|
|
||
| def parse_node_params( | ||
| kwargs: dict[str, Any], | ||
| ) -> tuple[dict[str, Any], dict[str, dict[str, Any]]]: | ||
| """Separate node parameters from other keyword arguments. | ||
|
|
||
| Node parameters are identified by containing a dot in their key. The part | ||
| before the first dot is considered the node name, the part after the dot | ||
| is the parameter name. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| kwargs : dict[str, Any] | ||
| Dictionary of keyword arguments. | ||
|
|
||
| Returns | ||
| ------- | ||
| tuple[dict[str, Any], dict[str, dict[str, Any]]] | ||
| Remaining keyword arguments and a dictionary of node parameters. | ||
| """ | ||
| remaining_kwargs = {} | ||
| node_params = {} | ||
|
|
||
| for key, value in kwargs.items(): | ||
| if "." in key: | ||
| node_name, param_name = key.split(".", 1) | ||
| if node_name not in node_params: | ||
| node_params[node_name] = {} | ||
| node_params[node_name][param_name] = value | ||
| else: | ||
| remaining_kwargs[key] = value | ||
|
|
||
| return remaining_kwargs, node_params | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"extra_args" is a bit vague, I would like to at least see some documentation on its intended purpose.