Skip to content

Fix prepare_data --data_dir support without --cluster - #1536

Open
Entity0-1 wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
Entity0-1:fix/local-prepare-data-dir
Open

Entity0-1 wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
Entity0-1:fix/local-prepare-data-dir

Conversation

@Entity0-1

@Entity0-1 Entity0-1 commented Oct 3, 2026 •

Copy link
Copy Markdown

Fixes #1036

Summary

Allow prepare_data to use --data_dir without an explicit --cluster.

The existing check prevents this combination. Removing that check alone
is insufficient because the subsequent copy step assumes container paths,
which are incorrect for execution directly on the host.

This change:

  • Uses host dataset paths for the none executor and preserves container
    paths for container execution.
  • Preserves cluster selection through NEMO_SKILLS_CONFIG.
  • Quotes dataset and copy paths, including paths containing spaces.
  • Keeps Slurm's requirement for data_dir unchanged.
  • Adds eight regression cases covering local execution, container paths,
    environment-selected clusters, and existing safeguards.

Testing

  • Focused and neighboring tests on Windows: 14 passed, 50 deselected.
  • Four new regression cases failed against the original implementation;
    all eight pass with this change.
  • Ruff formatting, scoped E/F/I lint, and whitespace checks passed.
    Full Ruff still reports five pre-existing findings in unchanged code.

The existing test_data_dir_collision_raises test was excluded because
native Windows path handling causes it to fail before reaching the
changed logic.

Job submission was mocked. Actual dataset preparation, Linux end-to-end
execution, Docker/Slurm execution, and full upstream CI have not been run.
d to implement the fix and write the regression tests.

Summary by CodeRabbit

  • Bug Fixes
    • Dataset paths containing spaces are now handled correctly during data preparation and copying.
    • Dataset copying now uses the appropriate path for local and container-based execution.
    • Specifying a data directory no longer requires a cluster setting. Slurm still requires a data directory.
  • Tests
    • Added coverage for path handling, executor behavior, cluster configuration, and data-directory options.

Signed-off-by: Nate <you@example.com>
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA-NeMo/Skills/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 0c7c6422-b0bb-4e74-a930-be84a0eb66b1
📥 Commits

Reviewing files that changed from the base of the PR and between 6e869c6 and cda765f.

📒 Files selected for processing (2)
  • nemo_skills/pipeline/prepare_data.py
  • tests/test_prepare_data.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/test_prepare_data.py
  • nemo_skills/pipeline/prepare_data.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

The prepare command shell-quotes dataset paths and copy paths. It selects the copy source based on the executor. Setting data_dir no longer requires an explicit cluster.

Changes

Prepare data command

Layer / File(s) Summary
Quote dataset command paths
nemo_skills/pipeline/prepare_data.py, tests/test_prepare_data.py
Dataset paths added to the prepare command are shell-quoted. Tests cover local and external dataset paths with spaces.
Select and quote copy paths
nemo_skills/pipeline/prepare_data.py, tests/test_prepare_data.py
The explicit-cluster validation is removed. Local execution uses the host dataset path as the copy source; other executors use the container dataset path. Tests cover executor selection, data-directory validation, and behavior without copying.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to cda76

The change appears mergeable after normal checks. No actionable risk to preparing data without an explicit cluster was established.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the main change: allowing prepare_data to use --data_dir without --cluster.
Linked Issues check ✅ Passed Issue #1036 requires prepare_data --data_dir to work without an explicit --cluster. The PR removes the rejection, uses host dataset paths for executor none, and retains container paths for conta…
Out of Scope Changes check ✅ Passed The implementation and tests support issue #1036. The changes since the previous review add docstrings to the affected code and tests. No unrelated change is evident.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: Nate <you@example.com>

This branch has not been deployed

No deployments
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.

Make sure data_dir works without --cluster parameter in prepare_data

1 participant