Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
the
documentation
![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: c0c7f1fd50
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DraftWorkerClass = self.spec_algorithm.create_worker(self.server_args) | ||
| self.draft_worker = DraftWorkerClass(**draft_worker_kwargs) |
There was a problem hiding this comment.
When a custom algorithm registered through SpeculativeAlgorithm.register is used with --speculative-draft-load-format, this path now passes the unchanged target ServerArgs to both its factory and worker. The new handling in draft_worker_common.py only covers the built-in workers, whereas this generic scheduler path previously set load_format before invoking any worker. Existing plugins that rely on server_args.load_format will therefore load the draft using the target format, which can make differently packaged draft checkpoints fail to load.
Useful? React with 👍 / 👎.
| "Falling back to grammar_backend='none'. " | ||
| "Structured outputs (JSON schema, regex, EBNF) will not be available." | ||
| ) | ||
| server_args.override("grammar.import_fallback", grammar_backend="none") | ||
| return None |
There was a problem hiding this comment.
When XGrammar rejects the model's tokenizer and strict thinking is disabled, this returns None but leaves the resolved configuration as grammar_backend="xgrammar". Consequently Scheduler.get_internal_state() and /server_info now report XGrammar even though structured-output requests are rejected because no backend exists; preserve the fallback in the runtime config bag so control-plane consumers see the backend actually in effect.
Useful? React with 👍 / 👎.
| prefill_default = None | ||
| if hybrid_gdn_config(runner.model_config) is not None and not is_npu(): | ||
| maybe_set_default_flashinfer_gdn_prefill(runner) | ||
| initialize_linear_attn_config(runner.server_args) | ||
| prefill_default = flashinfer_gdn_prefill_default(runner) | ||
| initialize_linear_attn_config(runner.server_args, prefill_default) |
There was a problem hiding this comment.
On an eligible SM100/CUDA 13 BF16 hybrid-GDN deployment without an explicit prefill backend, this selects FlashInfer only in the module-level linear-attention state. The runtime context still retains the startup value (linear_attn_prefill_backend=None, with the base typically triton), so get_internal_state() and /server_info no longer expose the backend the server is actually running, unlike the previous override-based behavior. Record this generated default in the resolved runtime configuration as well as passing it to the initializer.
Useful? React with 👍 / 👎.
c0c7f1f to
4cc8013
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffdbd267e5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # single_global_scale=True). ModelOpt's input_scale = amax / (6 * 448). | ||
| flat2 = scale2.reshape(-1).float() | ||
| if get_server_args().load_format == "dummy" and not bool( | ||
| if get_model().load_format == "dummy" and not bool( |
There was a problem hiding this comment.
When an Inkling multi-layer EAGLE deployment uses --speculative-draft-load-format=dummy while the target uses a normal load format, this reads the target process's get_model() bag rather than the draft ServerArgs copy created in Scheduler.maybe_init_draft_worker(). That copy is not installed into RuntimeContext while the v2 worker is constructed, so the dummy-only scale normalization is skipped and the following constant-scale assertion fails on the dummy loader's per-element noise, preventing the draft model from starting.
Useful? React with 👍 / 👎.
a5c0729 to
02c90ae
Compare
|
/tag-and-rerun-ci |
02c90ae to
159741d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 159741dc72
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.server_args.override( | ||
| "tokenizer.update_weights", model_path=model_path, load_format=load_format | ||
| ) | ||
| self.record_config_updates("tokenizer.update_weights", load_format=load_format) |
There was a problem hiding this comment.
After a successful update_weights_from_disk, this records only load_format while keeping the new model_path solely on the manager attribute. Both request-dump paths serialize the pristine self.server_args plus _config_updates, so a dump created after reloading another checkpoint now contains the startup model path and no metadata from which the active path can be recovered; previously the ServerArgs.override call placed the new path in the serialized object. Include the manager-owned model path in the dumped runtime snapshot or its update metadata so crash/request reproduction uses the checkpoint that actually produced the output.
Useful? React with 👍 / 👎.
159741d to
1795075
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17950751f0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "resolved_config": self.resolved_config_dict( | ||
| dataclasses.asdict(self.server_args) | ||
| ), |
There was a problem hiding this comment.
When ServerArgs contains an unpickleable dataclass field such as a locally defined custom_sigquit_handler, dataclasses.asdict() copies that callable into resolved_config. The first pickle.dump then fails as expected, but the existing retry only sets server_args to None, leaving the same callable in resolved_config, so the retry also raises and the request dump is lost. The crash-dump path adds the same snapshot and has the same regression; sanitize this dictionary or remove it during the fallback.
Useful? React with 👍 / 👎.
7f12342 to
5ac334c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac334caa6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| manager = _manager() | ||
| manager.model_path = "dummy" | ||
| manager.served_model_name = "dummy" | ||
| manager.server_args.custom_sigquit_handler = Hostile() |
There was a problem hiding this comment.
When this registered CPU test runs, _manager() has already constructed and materialized ServerArgs, whose __setattr__ rejects every post-resolution non-private assignment. This line therefore raises AttributeError before _dump_config_snapshot() is exercised, and the identical lambda assignment in the following test fails the same way; pass custom_sigquit_handler into _manager(...) during construction instead.
Useful? React with 👍 / 👎.
5ac334c to
49fa9b4
Compare
|
/tag-and-rerun-ci |
Each of these wrote a value after resolution so a later reader would find it on the instance. None of them needed the instance: one write was redundant, and the two that carry a value the resolved-config readback reports move to get_context().override, which the readback overlays. - The SM100 GDN prefill default was written onto ServerArgs and read back one line later by initialize_linear_attn_config. It is now the return value of flashinfer_gdn_prefill_default, threaded into initialize_linear_attn_config (an explicit --linear-attn-prefill-backend still wins) and recorded with get_context().override so /server_info reports the backend in effect. - The XGrammar fallback recorded grammar_backend="none" on the instance. No code reads the field after the factory reads it once, but get_internal_state reports the whole resolved config, so the fallback now lands there instead: the readback tells the truth and the seed keeps the requested backend. - UnifiedRadixCache.init_hicache re-applied the direct-IO layout fixup that __post_init__ already applies: init_hicache only runs when hierarchical cache is on, which is exactly when _handle_hicache normalizes page_first to page_first_direct (pinned by test_hicache_io_backend_and_mem_layout_ compatibility::direct_with_page_first). Three fixtures reached the fixup by building ServerArgs(model_path="dummy"), whose resolution is skipped, so they now declare the layout resolution would have produced. Writer ratchet 34 -> 31.
EAGLEWorkerV2, StandaloneWorkerV2, MultiLayerEagleWorkerV2 and FrozenKVMTPWorkerV2 wrote the draft's context_length onto the ServerArgs instance they share with the target worker, and the scheduler wrote the draft's load_format onto that same object just before creating them. The target's config carried draft values from then on, and anything constructed later in the process inherited them. Scheduler.maybe_init_draft_worker now makes one draft copy through draft_server_args_copy() and hands it to both the worker factory and the worker, so every algorithm gets it — the four built-ins, dflash/dspark (which deepcopy it again inside build_draft_tp_worker), and anything registered through SpeculativeAlgorithm.register. The copy starts from the config the process resolved, not from the pristine seed, so load-time overrides made before this point (the chunked-prefix gate, the SM100 GDN prefill default) are part of what the draft sees; context_length and load_format are applied on top. The construction runs under a preserved publish of that copy, the shape build_draft_tp_worker already used. Weight loading reads the bags rather than the instance it was handed — Inkling's ModelOpt scale normalization keys on load_format — so the draft has to be built with its own config published, and the target's is back in the slot when construction returns. The EAGLE hot-token-map write is deleted, not moved. init_token_map runs from alloc_memory_pool, long after the draft's TpModelWorker built its ModelConfig, and hot_vocab_size is only ever read off model_config.hf_config, which json_model_override_args reaches at ModelConfig construction. The write could not affect the draft model; only the shared instance saw it. hot_token_id is unchanged, so a draft checkpoint that declares hot_vocab_size behaves as before. Tests: draft_server_args_copy carries the target context_length, a configured draft load_format and any load-time override while leaving the target's instance alone; and the scheduler handoff pins that the factory and the worker both receive the copy, that the copy is the published config during construction, and that the target's is restored afterwards. Writer ratchet 31 -> 26.
The scheduler's runtime HiCache attach/detach wrote its own ServerArgs so the internal-state readback would show the change; that readback already reports the resolved config, so the writes become get_context().override(...) and the namespace readers see them too. The tokenizer side is per-engine — several Engines can share one process — so its control-plane updates (weight version, model path + load format, HiCache attach/detach) stay with the manager instead of moving to the process-global bags. TokenizerManager gains record_config_updates / config_value / resolved_config_dict, and the readbacks that used to observe the instance write (/server_info, /model_info, the HiCache status endpoint, the gRPC bridge) now overlay those updates onto the startup config. test_server_info's stub grew the real manager instead of a SimpleNamespace, so the overlay it now exercises cannot drift from production. Writer ratchet 26 -> 19.
… the bags When --forward-pass-metrics-ipc-name is left unset the reporter generates an endpoint and has to hand it to external consumers (the documented contract is that they read it back from the server config). That readback is the scheduler's get_internal_state, which already reports get_context().resolved_server_args_dict(), so the write moves to get_context().override and the read alongside it to get_observability() — the endpoint still shows up in /server_info's internal_states, and the ServerArgs instance stops being a message bus. The test's server_args stand-in (a SimpleNamespace with a hand-rolled override) becomes a real published config, so the reporter exercises the same accessors as production. Writer ratchet 19 -> 18.
`get_server_args().<field>` reads one process's startup record. Nine sites still did that for a value that has a namespace: the attention backend (5), `skip_tokenizer_init` (2), the draft-aware `load_format`, and a chunked-prefill size in `sglang.kernels`. They now read `get_exec().kernel` / `get_serving()` / `get_model()` / `get_schedule()`, so they see the resolved value including post-publish overrides. The multimodal processor's device selection moves to the instance it was constructed with rather than to a namespace: `base_gpu_id` differs per worker (the encode-server DP workers each specialise their own copy), so no process-global value can stand in for it, and engines sharing a tokenizer process each need their own. Branch order, the NPU preprocess patches, and the case that leaves "device" unset are unchanged. What stays on `get_server_args()` is the derived API — `@property` and method members computed from several fields plus the HF config (`mamba_cache_chunk_size`, `get_model_config()`, `enable_mamba_extra_buffer*`) — plus three config-intent reads of live-shadowed sizes, each of which needs an answer the live topology property cannot give (the DSA indexer's PP gate must short-circuit before touching the PP group, `allocation`'s DCP gate asks whether DCP was configured at all, and the CUDA-IPC recycler runs where no group exists). A new AST ratchet pins both shapes it can see — the direct call and an alias bound from it in the same function — at 0 and 12 respectively, exempting the derived APIs and those three sites by name. The alias-form baseline is not zero: those reads are mostly per-runner fields in model code, and lowering them is the next slice. Two fixtures stopped faking config: `test_dllm_fdfo_kv_reuse` rebound `allocation.get_server_args` to a SimpleNamespace, which silently stops intercepting the moment a reader migrates; it publishes a real config instead.
49fa9b4 to
e42cf37
Compare
|
All five members merged in order (each rebased onto main immediately before its merge):
Closing this review vehicle. On |
1 participant
Review/CI vehicle for the stacked series (full diff vs main; do not merge — merge the members in order):
ServerArgs.overridewrites that no reader needed (ratchet 34 → 31)ServerArgscopy instead of mutating the target's (31 → 26)ServerArgsinstance (26 → 19)Together: nothing writes config onto a published
ServerArgsfor a reader to find, and nothing reads config off the process-global instance when the value has a namespace.ServerArgsis back to being the startup record.The head carries one extra commit adding
STACK_REVIEW_PLACEHOLDER.md; drop it if this branch is ever squashed.Each member lowers its ratchet baseline itself, so every commit in the stack is green on its own — the ratchet tests were run at each commit.
Review round 1 — what changed
Nine findings, all real, all fixed in the originating commit:
SpeculativeAlgorithm.registerwould have loaded its draft in the target's format. The copy moved up intoScheduler.maybe_init_draft_worker, so it is made once for every algorithm — which also removes the local-rebinding footgun the second reviewer flagged (the workers no longer shadowserver_argswith a draft copy mid-__init__).get_internal_state//server_inforeport the whole resolved config, so both values silently stopped reflecting reality. Each is now recorded withget_context().override, which keeps the readback truthful without putting anything back on the instance. Both have a test asserting the value shows inresolved_server_args_dict()while the published instance stays pristine.model_pathwas dual-sourced (config: keep runtime hicache and weight-version updates off ServerArgs #33240): operational code readself.model_path, readbacks read the overlay dict.resolved_config_dictnow injects the manager's livemodel_path/served_model_name, so there is one store per value.record_config_updateswas an unvalidated free-form merge (config: keep runtime hicache and weight-version updates off ServerArgs #33240): it now rejects keys that are notServerArgsfields (a typo would otherwise surface as a phantom entry in/server_info) and keeps(source, fields)for provenance, like the context's override log.resolved_server_args_dictoverlaysvars(server_args), so an instance write would have kept it green. It now asserts throughget_observability()and that the published instance staysNone.sa = get_server_args(); sa.fieldescaped it, so_BASELINE = 0overstated the result. It is now scope-aware and package-wide, with an honest split: direct-form 0, alias-form 12 (mostly per-runner fields in model code), and the exempt derived APIs listed by name. The one package-level read outsidesrt(kernels/ops/layernorm/mhc.py) is flipped.OpenAIServingClassifyfreezingmodel_nameat construction is pre-existing and orthogonal, and extracting the manager's control-plane bookkeeping into its own collaborator is worth doing only if more fields accumulate.Validation
Full registered CPU battery (16 partitions) at the stack tip against the same battery on
main: identical failure sets, zero new failures. Per-area unit suites (mem_cache,constrained,layers/attention,spec,managers,entrypoints,observability,multimodal,server_args) and every config ratchet pass.What could not be validated locally, called out per PR as well:
Related: the per-role namespace enforcement shipped in #33172 was exercised on this branch (record + enforce modes, dp_size=2) and behaved as declared; no change here depends on it.
CI States
Latest PR Test (Base): 🚫 Run #30771349345
Latest PR Test (Extra): 🚫 Run #30771349277