Skip to content

Refactor workdir usage in Kubeflow and Kuberay - #501

Open
nathan-az wants to merge 9 commits into
NVIDIA-NeMo:mainfrom
nathan-az:nazrak/volume_mount_support
Open

Refactor workdir usage in Kubeflow and Kuberay#501
nathan-az wants to merge 9 commits into
NVIDIA-NeMo:mainfrom
nathan-az:nazrak/volume_mount_support

Conversation

@nathan-az

@nathan-az nathan-az commented May 1, 2026

Copy link
Copy Markdown

Motivation and Summary

The motivation behind this PR was to better support subPath and other volume mount options for the workdir. Due to a cloud limitation, our users share a PVC, and subPaths provide level of virtual isolation.

So the main changes:

  • users specify the dict for the mount itself rather than name and path
  • parameterise the optional internal subpath (e.g. {path}/{getuser}/"code")
  • keep this functionality in the executor rather than runner so the user can control it

kubeflow executor

  • changed workdir volume mount arg to accept dict
  • added new arg to specify filesystem subpath, defaulting to {path}/{getuser()}/"code" to avoid change in behaviour

kuberay executor

  • added workdir volume mount arg
  • added new arg to specify filesystem subpath, defaulting to {path}/{getuser()}/"code" to avoid change in behaviour

kuberay runner

  • removed the creation of the subpath, deferring instead to the functionality added in the executor.

Example Inputs

The pattern here changes to users explicitly specifying the VolumeMount they want for the workdir, expecting that volumes contains the relevant volume. e.g.

volumes = [{"name": "work-vol", "persistentVolumeClaim": {"claimName": "my-pvc"}}]
workdir_volume_mount = {"name": "work-vol", "mountPath": "/nemo_run", "subPath": team_name}

I haven't tested this yet on a real cluster, but will do so next week. PR is to get thoughts on the change in pattern.

@ko3n1g let me know if you have any thoughts on the pattern, or issues with the PR.

@nathan-az

nathan-az commented May 1, 2026

Copy link
Copy Markdown
Author

@ko3n1g it looks like the kuberay executor just uses the first entry in volume_mounts as the workspace dir. This is even simpler than what I've got here (albeit a bit less transparent). That said, since it's less verbose, I'm happy to swap to that pattern for kubeflow too, if you prefer it to the one in this PR. Or if you prefer the explicit choice/flexibility, can change kuberay to take an arg like in this PR (but in a follow-up PR).

@ko3n1g In order to keep the patterns consistent I have now made the same changes in each, and opted to increase flexibility in the executors. Default behaviour remains the same but with different args.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label May 3, 2026
@nathan-az
nathan-az force-pushed the nazrak/volume_mount_support branch from ce6c21f to 8f6691c Compare May 3, 2026 23:37
Nathan Azrak added 8 commits May 4, 2026 14:29
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
@nathan-az
nathan-az force-pushed the nazrak/volume_mount_support branch from 8f6691c to b02a015 Compare May 4, 2026 04:29
@nathan-az nathan-az changed the title Use explicit volume mount definition for kubeflow workdir Refactor workdir usage in Kubeflow and Kuberay May 4, 2026
Signed-off-by: Nathan Azrak <nazrak@gmail.com>
@ko3n1g

ko3n1g commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

/review

Copy link
Copy Markdown
Contributor

🔎 On it — resolving the pull request and reviewing it rigorously.

Copy link
Copy Markdown
Contributor

🤖 On it — handling this as review.

@ko3n1g

ko3n1g commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@nathan-az sorry letting this slip.. if you're still interested in landing this, please resolve the conflict and I'll get it merged either this or next week

Copy link
Copy Markdown
Contributor

💬 PR: #501
Verdict: Request changes. The workdir refactor has four blocking regressions, and the branch currently conflicts with main.

Blocking findings

  1. High — default Kubeflow launch scripts can abort before training. nemo_run/core/execution/kubeflow.py:150-152 now defaults code_dir to <mountPath>/<user>. Packaging creates that directory, after which nemo_run/core/execution/templates/kubeflow.sh.j2:14 runs ln -sfn <mountPath>/<user> /nemo_run. With the default /nemo_run mount, ln tries to replace the already-created /nemo_run/<user> directory and fails under set -e. I reproduced this ln failure. Remove or conditionally generate the symlink when the code directory is already under /nemo_run.

  2. High — KubeRay can sync code onto a volume that Ray containers never mount. nemo_run/run/ray/kuberay.py:1041-1052 gives workdir_volume_mount only to the data-mover pod, while KubeRayExecutor.get_cluster_body() still emits head and worker mounts solely from volume_mounts (nemo_run/core/execution/kuberay.py:175-197). The runner then assigns every container a workingDir beneath the unmounted path at nemo_run/run/ray/kuberay.py:1073-1087. Merge the selected mount into all applicable Ray pod specs or reject inconsistent configuration. The fixture at test/run/ray/test_kuberay.py:1222-1229 duplicates the mount in both fields and masks this case.

  3. High — Kubeflow can generate an invalid container mount list. nemo_run/core/execution/kubeflow.py:178-187 includes subPath in mount identity, allowing two entries with the same mountPath but different subpaths. Kubernetes requires container volumeMounts[].mountPath values to be unique. The new test at test/core/execution/test_kubeflow.py:505-532 explicitly expects two /nemo_run mounts, codifying a manifest the API will reject. Detect collisions by mountPath and replace, merge, or reject incompatible mounts.

  4. High — documented public configurations are broken without a migration path. nemo_run/core/execution/kubeflow.py:101-104 removes workdir_pvc and workdir_pvc_path, but docs/guides/executors/kubeflow.md:55-56 still passes both and will raise TypeError. Likewise, nemo_run/run/ray/kuberay.py:1032-1036 now rejects workdir unless the new field is set, while docs/guides/executors/kuberay.md:82-85,97-105 documents only volumes and volume_mounts. Add compatibility/deprecation handling or migrate all documented call sites and clearly declare the breaking change.

Non-blocking finding

  • Medium — the default remote layout no longer preserves existing behavior. nemo_run/core/execution/kubeflow.py:143-152 defaults to <mountPath>/<user>, whereas the prior layout and PR description specify <mountPath>/<user>/code. Consequently package/pull operations cover the whole user directory rather than the isolated code subtree. Preserve the /code suffix by default unless explicitly overridden.

Validation gaps

  • GitHub exposes only a passing DCO check for the PR head; no unit-test or lint check is visible.
  • A trial merge found a content conflict in nemo_run/core/execution/kubeflow.py.
  • git diff --check passed. Unit tests could not be run because this sandbox has no Python, uv, or pytest executable.
  • Add tests that execute the generated launch script, validate unique final mount paths, cover workdir_volume_mount without duplication in volume_mounts, and verify worker-group mounts and backward-compatible constructor behavior.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond and removed waiting-on-maintainers Waiting on maintainers to respond labels Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request waiting-on-customer Waiting on the original author to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants