Repository navigation
[fix] Let concurrent jobs keep their TransferQueue actors apart with TQ_RAY_NAMESPACE - #206
nataliekung wants to merge 2 commits into
Conversation
…TQ_RAY_NAMESPACE The controller, the Ray storage actor and the lock managers are all named actors in the fixed Ray namespace transfer_queue. Two jobs on one Ray cluster therefore attach to the same controller: the second job reads the first job's partitions, and the first job's close() kills the controller the second job is using. Read the namespace from TQ_RAY_NAMESPACE and keep transfer_queue as the default, so cross-job sharing (Ascend#115) is unchanged. Signed-off-by: nataliekung <4054699+nataliekung@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aa16420bb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Defaults to ``transfer_queue``, shared by every job on the Ray cluster (see #115). Set | ||
| ``TQ_RAY_NAMESPACE`` in every process of a job to keep concurrent jobs apart. | ||
| """ | ||
| return os.environ.get("TQ_RAY_NAMESPACE") or "transfer_queue" |
There was a problem hiding this comment.
When TQ_RAY_NAMESPACE is set for a run of the shipped streaming tutorial or relax demo, tq.init() now creates or attaches in that namespace, but a repo-wide namespace search still finds hard-coded namespace="transfer_queue" lookups in tutorial/06_streaming_dataloader.py:187 and recipe/simple_use_case/relax_demo.py:134. Those workers therefore either raise ValueError when the default namespace is empty or read another job's controller, defeating the advertised isolation; migrate these supported callers to ray_namespace() as well.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
…relax demo Both look up the controller by name with the hard-coded transfer_queue namespace, so they would miss the controller once TQ_RAY_NAMESPACE is set. Signed-off-by: nataliekung <4054699+nataliekung@users.noreply.github.com>
|
The review finding is valid: the streaming tutorial and the relax demo still looked the controller up in the hard-coded |
Problem
The controller,
RayObjectRefStorageand theTransferQueueLockManager_*actors are named actors in the fixed Ray namespacetransfer_queue. Two jobs on one Ray cluster (for example tworay job submitruns of a trainer) therefore attach to the same controller:tq.init()finds the first job's controller and reads its partitions;tq.close()kills that controller while the second job still uses it.#115 made the namespace fixed on purpose, so a trainer job can attach to TransferQueue created by another job. That behaviour is kept; this PR only makes the namespace selectable.
Change
ray_namespace()inutils/common.pyreturnsTQ_RAY_NAMESPACE, ortransfer_queuewhen it is unset or empty. It replaces the literal at all six sites (controller create and lookup, Ray storage actor create and lookup, lock manager create and lookup). A job that must stay isolated sets the variable to a per-job value in every process (for example through the job-levelruntime_envenv_vars). A short README paragraph documents it.The default is unchanged, so nothing changes for existing users.
Evidence
Two drivers on one local Ray head (
ray start --head), the first one puts("p", "k")and stays alive, the second one callstq.kv_list("p"):TQ_RAY_NAMESPACE{'p': {'k': {}}}(the first job's data)TQ_RAY_NAMESPACE=job-a/job-b{}Tests
tests/test_ray_namespace.py: default and empty value fall back totransfer_queue; the env value is returned; withTQ_RAY_NAMESPACE=job-a,tq.init()creates the controller injob-aand not intransfer_queue.pytest tests/test_ray_namespace.py tests/test_kv_lock.py tests/test_storage_client_factory.py tests/e2e/test_kv_global_lock_e2e.py: 34 passed, 2 skipped, Ray 2.57.0, torch 2.13, Python 3.11.ruff checkandruff format --checkclean.