Skip to content

Fix HybridPackager failing on paths with spaces or apostrophes - #646

Merged
ko3n1g merged 1 commit into
NVIDIA-NeMo:mainfrom
rioyu123:codex/fix-hybrid-packager-paths
Oct 9, 2026
Merged

ko3n1g merged 1 commit into
NVIDIA-NeMo:mainfrom
rioyu123:codex/fix-hybrid-packager-paths

Conversation

@rioyu123

@rioyu123 rioyu123 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Summary

HybridPackager fails when the job directory, archive name, or a sub-packager's archive path contains a space or an apostrophe. Those paths are inserted unquoted into the tar, gzip, and rm commands: spaces split them into separate arguments, while apostrophes break shell parsing.

This quotes each file path with shlex.quote(), as #637 and #645 did for PatternPackager and GitArchivePackager. Otherwise the commands are unchanged. Both extract_at_root layouts and the cleanup of temporary files and sub-archives work as before.

Folder names inside the tar transform expressions and executor extraction commands are outside this change.

Tests

  • Added 12 regression cases covering both path characters, all three path locations, and both layouts. Sub-packagers are mocks returning real archives; tar, gzip, and rm run for real. The test checks the combined archive's file names and contents, and removal of temporary files and sub-archives.
  • All 12 cases fail on unmodified main; the 2 existing hybrid tests pass. With the fix, all 42 packaging tests pass.
  • Full suite on Linux with Python 3.11.9 and locked dependencies: 1,712 passed, 1 deselected, 1 xfailed. Run with a fresh /tmp mount to isolate host temporary files. Pytest exited successfully; Ray then printed an ignored ModuleNotFoundError from its shutdown hook.
  • Full-repository Ruff lint and formatting checks and git diff --check pass.
  • Not run on macOS; the BSD tar transform expressions are unchanged.

@copy-pr-bot

copy-pr-bot Bot commented Oct 9, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ko3n1g

ko3n1g commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

/ok to test 08ee182

@ko3n1g

ko3n1g commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

/review

@svcnvidia-nemo-ci svcnvidia-nemo-ci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No merge-blocking defects found.

HybridPackager consistently quotes command path operands while preserving both archive layouts and cleanup behavior. The 12 regression cases cover spaces and apostrophes in all three targeted path locations, checking archive names, contents, and temporary-file removal.

Compared both changed files against base and inspected the sub-packager contracts, command runner, and an executor caller. Static review only; tests and platform-specific commands were not executed.

Review generated by codex

@ko3n1g
ko3n1g added this pull request to the merge queue Oct 9, 2026
Merged via the queue into NVIDIA-NeMo:main with commit be41071 Oct 9, 2026
31 checks passed

This branch was successfully deployed

1 active deployment
public — 08ee1828 Deployed Oct 9, 2026 by copy-pr-bot[bot] via release / finalize / publish-docs #403
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants