feat: node param override#56
Conversation
ndahn
left a comment
ndahn
left a comment
There was a problem hiding this comment.
Thank you @TomCC7, and sorry for the exceptionally long wait :') I had a few comments on the implementation, let me know if you find time to address them. There's a lot of inconsequential format changes which is not very nice, but fine - these won't stop me from accepting your PR.
There was a problem hiding this comment.
Don't think this should be part of this PR
| ctx.command.allow_extra_args = True | ||
| ctx.allow_extra_args = True | ||
|
|
||
| def _bool_param_type(value: str) -> bool: |
There was a problem hiding this comment.
click already handles bool types well, don't think this is needed
| 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 |
There was a problem hiding this comment.
I don't like having this on by default, is this needed for this PR?
| def launch_toml( | ||
| path: str, | ||
| launch_args: dict[str, str] = None, | ||
| extra_args: list[str] = None, |
There was a problem hiding this comment.
"extra_args" is a bit vague, I would like to at least see some documentation on its intended purpose.
| # We don't want to log this | ||
| print(msg) | ||
|
|
||
| def set_node_param_overrides(self, overrides: dict[str, dict[str, Any]]) -> None: |
There was a problem hiding this comment.
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.
| launch_args = {k: v for k, v in kwargs.items() if not k.startswith("bl_")} | ||
|
|
||
| # Pass current bl_* settings as CLI args to child process | ||
| from better_launch.utils.settings import Settings |
| args.extend(["--bl-node-param-override", "true"]) | ||
|
|
||
| # Pass through extra args (e.g., --node.param value for node overrides) | ||
| args.extend(ctx.args) |
There was a problem hiding this comment.
This feels wrong, the function is already executed by click with whatever CLI args it received.
| # of a point to this. | ||
| # TODO pass overrides? | ||
| launch_toml(self.filepath, launch_args=kwargs) | ||
| launch_toml(self.filepath, launch_args=launch_args, extra_args=ctx.args) |
There was a problem hiding this comment.
This feels wrong, the function is already executed by click with whatever CLI args it received.
There was a problem hiding this comment.
Check if the examples/param_echo_node.py I recently included could be used or extended here instead.
| script_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| bl.node( | ||
| package=".", | ||
| executable=os.path.join(script_dir, "scripts", "timed_talker.py"), |
There was a problem hiding this comment.
Really not necessary, just use bl.find()
2 participants
Hi, this is an reimplementation of my previous PR #49 based on the new project state. I've tested locally with the example and it's working correctly. Closes #44 .