Skip to content

Conversation

@ji-huazhong
Copy link
Collaborator

What does this PR do?

Add concise overview of what this PR aims to achieve or accomplish. Reference related GitHub issues and PRs that help with the review.

Checklist Before Starting

  • Search for similar PRs. Paste at least one query link here: ...
  • Format the PR title as [{modules}] {type}: {description} (This will be checked by the CI)
    • {modules} include fsdp, megatron, sglang, vllm, rollout, trainer, ci, training_utils, recipe, hardware, deployment, ray, worker, single_controller, misc, perf, model, algo, env, tool, ckpt, doc, data
    • If this PR involves multiple modules, separate them with , like [megatron, fsdp, doc]
    • {type} is in feat, fix, refactor, chore, test
    • If this PR breaks any API (CLI arguments, config, function signature, etc.), add [BREAKING] to the beginning of the title.
    • Example: [BREAKING][fsdp, megatron] feat: dynamic batching

Test

For changes that can not be tested by CI (e.g., algorithm implementation, new model support), validate by experiment(s) and show results like training curve plots, evaluation results, etc.

API and Usage Example

Demonstrate how the API changes if any, and provide usage example(s) if possible.

# Add code snippet or script demonstrating how to use this

Design & Code Changes

Demonstrate the high-level design if this PR is complex, and list the specific changes.

Checklist Before Submitting

Important

Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.

@0oshowero0 0oshowero0 requested a review from Copilot October 15, 2025 09:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the transfer queue functionality by consolidating duplicate code into a shared base worker class and making the TransferQueue dependency optional through extra requirements.

  • Moves duplicate create_transferqueue_client method from specialized worker classes to the base worker class
  • Makes TransferQueue an optional dependency via extras_require instead of a mandatory requirement
  • Removes specialized worker wrapper files and updates imports to use the original worker classes directly

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
verl/single_controller/base/worker.py Added create_transferqueue_client method to base worker class
setup.py Moved TransferQueue from required to optional dependencies under transferqueue extra
requirements_transferqueue.txt Added dedicated requirements file for TransferQueue dependency
recipe/transfer_queue/megatron_workers.py Removed wrapper file containing duplicate worker classes
recipe/transfer_queue/main_ppo.py Updated imports to use original worker classes instead of wrapper classes
recipe/transfer_queue/fsdp_workers.py Removed wrapper file containing duplicate worker classes

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@0oshowero0 0oshowero0 merged commit 340bd40 into TransferQueue:main_tq_submodule Oct 15, 2025
2 of 4 checks passed
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.

2 participants