[core][uv] Keep the command intact when it has "--option=value" arguments - #65081
[core][uv] Keep the command intact when it has "--option=value" arguments#65081gouravmohanty7070 wants to merge 1 commit into
Conversation
…ents The runtime env hook works out which arguments belong to 'uv run' by parsing them with an optparse parser and treating the first argument the parser does not recognize as the start of the command. It then recovers the 'uv run' arguments by subtracting the length of that command from the original command line. That subtraction assumes the command the parser hands back is a suffix of the arguments it was given, which is not true when the first unrecognized argument is written as "--option=value": optparse splits the argument at the "=" and pushes the bare "value" back onto the remaining arguments before it reports the option as unknown. The command then comes back one entry longer than it should, so the 'uv run' arguments are cut one argument short. For "uv run -m ray.util.client.server --address=host:port ..." this yields "uv run -m", and the following argparse call fails with "argument -m/--module: expected one argument". Ray Client passes all of the arguments of its server in this form, so 'uv run' together with Ray Client fails this way. Make _parse_args return a command that is always a suffix of the arguments it was given by locating the unrecognized argument in them, which also lets the caller drop the argument counting. Fixes ray-project#62650 Signed-off-by: Gourav Mohanty <gouravmohanty2001@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where command arguments formatted as --option=value cut the uv run arguments short by refactoring _parse_args to correctly locate the boundary of unrecognized arguments and updating the argument extraction logic. It also adds comprehensive unit tests for these edge cases. The review feedback points out a potential bug where the boundary index could decrement to -1 if the unrecognized option is the first argument, which would cause incorrect slicing, and suggests safeguarding it with max(0, boundary - 1).
| if not (rargs and args[boundary] == f"{err.opt_str}={rargs[0]}"): | ||
| # The argument was removed entirely rather than split at the "=". | ||
| boundary -= 1 |
There was a problem hiding this comment.
In Python, a negative index in a slice like args[boundary:] is relative to the end of the list (e.g., args[-1:] returns only the last element). If boundary is 0 (which can happen if the unrecognized option is the very first argument) and it gets decremented, it becomes -1, causing the slice to incorrectly return only the last element of the command instead of the entire list.
To prevent this slicing bug, ensure that boundary never drops below 0 by using max(0, boundary - 1).
| if not (rargs and args[boundary] == f"{err.opt_str}={rargs[0]}"): | |
| # The argument was removed entirely rather than split at the "=". | |
| boundary -= 1 | |
| if not (rargs and args[boundary] == f"{err.opt_str}={rargs[0]}"): | |
| # The argument was removed entirely rather than split at the "=". | |
| boundary = max(0, boundary - 1) |
Description
uv runcurrently breaks whenever the first argument of the wrapped command is written as--option=value.The runtime env hook has to split the driver's command line into the part that belongs to
uv runand the part that is the actual command. It does that by parsing the command line with anoptparseparser that knows theuv runoptions, treating the first argument the parser doesn't recognize as the start of the command, and then recovering theuv runarguments by subtracting the length of that command from the original command line.That subtraction assumes the command handed back by
_parse_argsis a suffix of the arguments it was given. It isn't, when the first unrecognized argument uses the--option=valueform:optparse._process_long_optpops the argument, splits it at the=, pushes the barevalueback onto the remaining arguments, and only then reports the option as unknown. The command therefore comes back one entry longer than the slice it is measured against, and theuv runarguments are cut one argument short:uv_run_argsbeforeuv_run_argsafteruv run -m ray.util.client.server --address=h:puv run -muv run -m ray.util.client.serveruv run -m ray.util.client.server --address h puv run -m ray.util.client.serveruv run --model=x script.pyuvuv runThe truncated
uv run -mthen reaches theargparsecall just below, which fails with the error from the issue:Ray Client passes every argument of its server in the
--option=valueform (--address=...,--host=...,--port=...,--mode=...), souv runcombined with Ray Client fails this way every time.Related issues
Fixes #62650
Additional information
_parse_argsnow returns a command that is always a suffix of the arguments it was given, by locating the unrecognized argument in the original list instead of rebuilding it fromerr.opt_str. The caller then derives the boundary directly from the command's length, which removes theoriginal_length/args_consumedbookkeeping.Two details worth flagging for review:
err.opt_strcannot recover the original argument, because after the exception the remaining arguments look identical for--address=h:pand--address h:p. So the boundary is decided by checking whether the original argument at that position is exactlyf"{err.opt_str}={rargs[0]}". Comparing the whole reconstructed argument rather than usingstartswithmatters for a case like--model --model=x, where the value of a space-separated unknown option itself looks likeoption=value._parse_argsnow copies the list it is given instead of parsing it in place, so the caller no longer has to snapshot its length before parsing.Testing
test_uv_run_parsergains the--option=valueforms, including an empty value, an unknown option in that form directly afteruv run, and the--model --model=xcase above.test_uv_run_runtime_env_hook_option_with_equalsis a new test covering the reported failure at thehook()level, using a Ray Client style command line. It stubs_get_uv_run_cmdline, so it needs neither a cluster nor a realuvprocess.Both tests fail on master (the new one with the exact
argument -m/--module: expected one argumentabove) and pass with this change:I also replayed every command line the existing tests in this file use through the old and the new boundary logic and confirmed they agree on all of them, so the change only affects the
--option=valuecase that was broken.ruff check,ruff formatandpydoclintare clean on both files. The other tests in this file need a Ray cluster or a realuvsubprocess, which my sandbox can't provide (psutil.pids()is blocked), so I'm relying on CI for those.