Skip to content

fix(sglang): expose cfg on SGLangGeneration - #3516

Closed
tianyi-zhang-02 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:fix/sglang-generation-cfg
Closed

fix(sglang): expose cfg on SGLangGeneration#3516
tianyi-zhang-02 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:fix/sglang-generation-cfg

Conversation

@tianyi-zhang-02

@tianyi-zhang-02 tianyi-zhang-02 commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds a read-only cfg property to SGLangGeneration, aliasing its existing sglang_cfg.

create_weight_synchronizer reads generation.cfg before backend dispatch. Without this property, colocated SGLang raises AttributeError instead of constructing its HTTPWeightSynchronizer.

Testing

  • Added a CPU-only factory regression test using the real SGLangGeneration class
  • Test passes locally
  • ruff check and ruff format pass

Issues

Found while investigating #3288. Draft PR #3330 contains the same property as part of a larger change.

cc @RayenTian @yuki-97

@tianyi-zhang-02
fix(sglang): expose cfg on SGLangGeneration
ec452d7 
create_weight_synchronizer reads generation.cfg unconditionally, before any
backend dispatch. SGLangGeneration stored the generation config only as
self.sglang_cfg and defined no cfg attribute, so backend=sglang raised
AttributeError there instead of getting its HTTPWeightSynchronizer.

VllmGeneration, TRTLLMGeneration and MegatronGeneration all expose cfg;
megatron_generation.py documents it as the GenerationInterface contract.
sglang_cfg already is that same config object, so alias it with a read-only
property rather than keep a second reference that could drift.

Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
@tianyi-zhang-02
tianyi-zhang-02 requested review from a team as code owners August 6, 2026 07:23
@copy-pr-bot

copy-pr-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@tianyi-zhang-02 tianyi-zhang-02 mentioned this pull request Aug 6, 2026
Open
@tianyi-zhang-02

tianyi-zhang-02 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Closing this in favour of #3519, which carries the identical commit as its first commit — the cfg property and test_colocated_sglang_accepts_real_generation_object are both in that branch verbatim, so nothing is lost by closing this and it is one less PR in the queue.

The underlying bug is unchanged and still worth fixing: create_weight_synchronizer reads generation.cfg before any backend dispatch (nemo_rl/weight_sync/factory.py:69), while SGLangGeneration only sets self.sglang_cfg, so the single-controller path raises AttributeError for backend=sglang. vLLM, TRT-LLM and Megatron all expose cfg; megatron_generation.py:96 documents it as the GenerationInterface contract.

Happy to reopen as a standalone five-line PR if you would rather land it separately from the checkpoint-engine work — just say so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

community-request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@tianyi-zhang-02