Skip to content

convert_v3_to_v2: allow dataset paths under root's home - #763

Open
rakhimovv wants to merge 1 commit into
NVIDIA:mainfrom
rakhimovv:fix/convert-v3-v2-root-home
Open

rakhimovv wants to merge 1 commit into
NVIDIA:mainfrom
rakhimovv:fix/convert-v3-v2-root-home

Conversation

@rakhimovv

Copy link
Copy Markdown

Thanks for shipping the SO-100 conversion script — this turned up while running it as root in a container.

Fixes #

Changes proposed in this pull request:

  • Drop /root from the system_dirs blocklist in _validate_video_paths

What happens

_validate_video_paths lists /root alongside genuine system directories:

system_dirs = {"/etc", "/sys", "/proc", "/dev", "/boot", "/root"}

/root is root's home directory. --root defaults to None, in which case L491 falls back to HF_LEROBOT_HOME / repo_id, which for a root user with no HF_HOME override is /root/.cache/huggingface/lerobot/.... So the script's default invocation rejects every video it is asked to convert.

Measured on 51d4c89, running as root, HF_HOME and friends unset:

$ uv run --project scripts/lerobot_conversion \
    python scripts/lerobot_conversion/convert_v3_to_v2.py --repo-id izuluaga/finish_sandwich

  File ".../convert_v3_to_v2.py", line 317, in _extract_video_segment
    _validate_video_paths(src, dst)
  File ".../convert_v3_to_v2.py", line 298, in _validate_video_paths
    raise ValueError(f"Path points to system directory: {name} path {resolved_path}")
ValueError: Path points to system directory: source path
/root/.cache/huggingface/lerobot/izuluaga/finish_sandwich/videos/observation.images.wrist/chunk-000/file-000.mp4

Metadata and parquet conversion complete first; it dies at "Converting concatenated MP4 files back to per-episode videos".

The same thing happens on the README path when the checkout is under root's home — examples/SO100/README.md line 10 suggests export GR00T_REPO=~/Isaac-GR00T, which for root makes the relative --root examples/SO100/finish_sandwich_lerobot resolve to /root/Isaac-GR00T/.... I have not run that variant; the traceback above is the one I measured.

Running as root is the default in a container, and #653 reports that the repo's own Dockerfile has no USER directive.

Why remove the entry rather than make it conditional

I first tried exempting /root when it is the invoking user's home, and that predicate is wrong: Path.home() resolves $HOME before the passwd entry, so HOME=/root with a non-root user drops the guard, while sudo with HOME=/home/alice keeps blocking the case this is meant to fix. Keying on os.geteuid() instead would introduce a mechanism this repo uses nowhere. A blocklist entry whose effect depends on an environment variable seemed worse than either constant, so this PR just removes it. The other five entries are untouched.

One thing I'd flag rather than change

These paths are not purely operator-supplied. L506 derives video_keys from info["features"] of a dataset fetched by snapshot_download, and those strings are interpolated into the source and destination paths, so a hostile repo-id influences where the script reads and writes. The comment at L282 says "Check for path traversal attempts in the original paths", but the code under it checks null bytes and control characters only.

A containment check — asserting src_resolved is under root and dst_resolved under new_root — would cover that properly and make the whole system_dirs list redundant, /root included. That is a bigger change to your security posture than I'd want to make uninvited, so I've left it out of this PR; happy to open it separately if you'd like it.

Verification

Same command, same environment, before and after on this branch:

result
51d4c89 rc=1, ValueError above
this branch rc=0, both camera streams converted

tests/examples/test_so100.py::test_so100_readme_workflow_executes_via_subprocess also passes end to end with the change (18m12s, one H100). ruff check and ruff format --check clean.

Before submitting

  • I've read and followed all steps in the Making a pull request section of the CONTRIBUTING docs — that anchor 404s for me; CONTRIBUTING.md currently has # Contributions and ## Support only, so I've followed its "submit a pull request" line. Tell me if there's a step I've missed.
  • I've updated or added any relevant docstrings — comment added recording why /root is absent, so it doesn't get re-added.
  • If this PR fixes a bug, I've added a test that will fail without my fix — tests/examples/test_so100.py covers it end to end but needs a GPU. A focused unit test would have to live under scripts/lerobot_conversion, which has no test module today; say the word and I'll add one there.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality — n/a.

The default dataset root (HF_LEROBOT_HOME) resolves under /root when the
process runs as root, so the system-directory guard rejected every video path
and the script's default invocation could not complete.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant