[Cleanup] [Policy] Ensure Policy is documented and matches vLLM #382
Conversation
meta-cla
bot
added
the
CLA Signed
| trace_headers=None, | ||
| priority=priority, | ||
| data_parallel_rank=None, | ||
| data_parallel_rank=None, # We do not support DP |
There was a problem hiding this comment.
is it possible someone tries to set DP through the vLLM args? Or in other words, any possibility this fails silently because this is a comment instead of an error?
There was a problem hiding this comment.
Haha there's a lot of that b/c we choose not to have a shim layer between what we support and the span of config options offered through vLLM.
I can try to call out the egregious cases like this one though.
There was a problem hiding this comment.
ok fair haha
I think this isn't as important for today, and maybe we can cover ourselves by just commenting that our design choice broadly comes with implications that things don't work / this is experimental...
There was a problem hiding this comment.
we choose not to have a shim layer between what we support and the span of config options
This is the right move for simplifying things, I'd be shocked if we don't end up talking about adding shim back by EOY haha
There was a problem hiding this comment.
I think it'd be easy to add an assert in the post init
Co-authored-by: Jack-Khuu <jack.khuu.7@gmail.com>
Co-authored-by: Jack-Khuu <jack.khuu.7@gmail.com>
| TORCHSTORE_USE_RDMA.get_value() == 0 | ||
| ) # torchstore currently only accepts 0 or 1 | ||
| # Gets set up by setup | ||
| # Remaining variables are initialized in self.setup() |
There was a problem hiding this comment.
This shouldn't be necessary. Everything up here are config like objects. So dataclasses, direct values, or callables. You shouldn't have to specify that variables are created in setup as that's the pattern everywhere. Also, I'd be fine with removing lora_request if we don't support it now.
There was a problem hiding this comment.
Yeah fair, just didn't know if you had left this in here for readability at a glance.
| prompt_str, request = self.processor.process_inputs( | ||
| request_id=request_id, | ||
| prompt=prompt_dict, | ||
| prompt={"prompt": prompt}, |
There was a problem hiding this comment.
This is probably fine but it takes us a bit further from vllm
| trace_headers=None, | ||
| priority=priority, | ||
| data_parallel_rank=None, | ||
| data_parallel_rank=None, # We do not support DP |
There was a problem hiding this comment.
I think it'd be easy to add an assert in the post init
|
|
||
| @dataclass | ||
| class PolicyWorker(ForgeActor): | ||
| """Mirrors a vLLM GPUWorker |
There was a problem hiding this comment.
We could make this match the parent worker so that we can just pipe the args, kwargs though in launch and then build vllm config here as well.
5 participants
Context
(See specific changes & comments in the PR files themselves)
How do you know I didn't break things?
python tests/integration_tests/test_vllm_policy_correctness.pyResults:
python -m apps.grpo.main --config apps/grpo/qwen3_1_7b.yamlResults: https://wandb.ai/jcummings/grpo-training/runs/kt1m006u?nw=nwuserjcummings