google-deepmind / google-deepmind/xmanager

`ARG_ESCAPER` shell-quotes arguments for direct `argv` backends

Open
#75 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
921
Forks
69
Avg merge
1d 1h
Merged PRs (30d)
1

Description

Hello!

I expected a job argument value to reach a job unchanged across executors, but observed that values requiring posix shell quoting arrive with literal `'` characters on `LOCAL_DOCKER`, `Vertex`, and `Kubernetes` executor. I see this causing, for example, a json serialized arguments failing.

I believe the problem is that `xm.utils.ARG_ESCAPER` applies `shlex.quote` while `SequentialArgs.to_list` constructs the argument list. This works for the local-binary path, where `asyncio.create_subprocess_shell` removes the quoting. However, the container backends pass each resulting token directly as `argv`, so nothing removes the quoting.

The `tmux` path has a different but related issue: it applies `subprocess.list2cmdline` to the shell-quoted tokens and then passes the result to a POSIX shell. `subprocess.list2cmdline` implements Windows quoting rules, so the two quoting schemes do not work together.

The exec-form Docker `ENTRYPOINT` path also constructs its json array by wrapping each argument in `"`. An argument containing `"` can therefore produce an invalid `Dockerfile`.

Here's a minimal reproduction:

Reproduction

```python
import subprocess
import sys

from xmanager import xm
from xmanager.xm import utils

value = '{"d": 4}'
arguments = xm.merge_args(
xm.SequentialArgs(),
{'config': value},
).to_list(utils.ARG_ESCAPER)

print(arguments)
subprocess.run(
[
sys.executable,
'-c',
'import sys; print(repr(sys.argv[1]))',
*arguments,
],
check=True,
)
```

```console
$ python arg_escaping_repro.py
['--config=\'{"d": 4}\'']
'--config=\'{"d": 4}\''
```

The job receives `'{"d": 4}'` rather than `{"d": 4}`.

I can work around this by using `ShellSafeArg` only for container backends. The annoying thing is that `ShellSafeArg` suppresses quoting for the shell path as well, so there is no single argument representation that works across executors.

I think the serialization needs to distinguish direct `argv` consumers from commands interpreted by a shell. I have a patch that I can open a PR with in a bit.

Contributor guide

Open the contributing guide

Research direction

Start by tracing xm.utils.ARG_ESCAPER through SequentialArgs.to_list, then inspect the local-binary, container, tmux, and Docker ENTRYPOINT paths described in the issue. Verify the argument representation used by each executor with the supplied reproduction. Done means raw values reach direct argv consumers, shell consumers remain correctly quoted, and arguments containing double quotes produce valid Docker JSON.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.