config: the lazy imports that buy nothing become eager - #36975
Merged
Conversation
ch-wan
requested review from
BBuf,
CatherineSue,
ClawSeven,
DarkSharpness,
Edwardf0t1,
Fridge003,
HaiShaw,
HydraQYH,
Qiaolin-Yu,
Ying1123,
alphabetc1,
b8zhong,
celve,
hebiao064,
hnyls2002,
iforgetmyname,
ispobock,
jybsuper,
lifuhuang,
merrymercy,
mmangkad,
ping1jing2,
slin1237,
whybeyoung,
yuan-luo and
yushengsu-thu
as code owners
August 29, 2026 06:21
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. |
This was referenced Aug 29, 2026
ch-wan
force-pushed
the
cheng/gc-r4e-eager-imports
branch
from
August 29, 2026 07:13
24e78d7 to
81c7523
Compare
ch-wan
requested review from
AgainstEntropy,
ByronHsu,
Duyi-Wang,
Kangyan-Zhou,
OrangeRedeng,
ShangmingCai,
bingxche,
mickqian and
yichiche
as code owners
August 29, 2026 07:13
ch-wan
force-pushed
the
cheng/gc-r4d-dead-params
branch
2 times, most recently
from
August 29, 2026 07:37
5a127e6 to
22962c3
Compare
ch-wan
force-pushed
the
cheng/gc-r4e-eager-imports
branch
from
August 29, 2026 07:37
81c7523 to
5284d7f
Compare
ch-wan
force-pushed
the
cheng/gc-r4d-dead-params
branch
from
August 29, 2026 08:11
22962c3 to
64840a1
Compare
ch-wan
force-pushed
the
cheng/gc-r4e-eager-imports
branch
from
August 29, 2026 08:11
5284d7f to
ceb011e
Compare
ch-wan
force-pushed
the
cheng/gc-r4d-dead-params
branch
from
August 29, 2026 08:32
64840a1 to
639f853
Compare
ch-wan
force-pushed
the
cheng/gc-r4e-eager-imports
branch
from
August 29, 2026 08:32
ceb011e to
a04c86f
Compare
`arg_groups.overrides` is imported at module scope by thirty-five files already, `server_args` among them, so importing it is not a cycle and not a cost -- by the time any resolution step runs, the module is loaded. Ninety-three `from ...overrides import` statements were nonetheless written inside the functions that use them. The ones that buy nothing become module-scope imports, including the twenty-three that were **mixed**: a name a test patches, plus names riding along on the same statement that nothing patches. Splitting those frees the riders -- the `_dsa_*` pass callables are the shape of it, only ever handed to `run_post_process_pass` as arguments, never looked up through the module. Three groups were kept lazy for a test seam, which is the wrong way round, and each is fixed at the test rather than in the code the runtime executes: - The four declaration paths. `test_no_entry_changes_after_it_is_recorded` wrapped `declare_resolution`, `declare_late_resolution`, `declare_direct_writes` and `run_post_process_pass` by `setattr` on the module, purely to snapshot the stash after each append. The property it checks belongs to the stash, so the seam is now the stash: a list that snapshots on append, installed by a test-local `ServerArgs` subclass that survives the pipeline's own reset of it. The guard is strictly stronger -- it records at the append rather than after the call -- and still fails on an injected in-place edit. - `use_mla_backend`. Eight cases patched the answer; it is `model_config_of(server_args).attention_arch == AttentionArch.MLA`, so the fixtures now state the model instead. Five of the patches were redundant outright -- their stand-in already answered "not MLA" -- and the rest set `_model_config.attention_arch`, which is the input the value is derived from. - `model_config_of` had no patch site at all and was lazy on a hunch. What is left is load-bearing, for exactly two reasons and nothing else: `attention_backends_of` (3 patch sites) and `supports_mamba_cache_extra_buffer` (2) are still patched through the module, and `runtime_context` is imported by `utils.common`, which `overrides` needs, so it sits *below* `overrides` and cannot import it at module scope -- a cycle, not a preference. A function-local `overrides` import that is neither of those: zero. A dummy resolution is unchanged at ~9 ms and loads the same six `arg_groups` modules; the 62-shape resolution probe is byte-identical to the base commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The series
Stacked, each PR based on the one above it. Read them in order; every boundary is
self-sufficient and green on its own.
main, not for mergeMotivation
arg_groups.overridesis already imported at module scope by thirty-five files,server_argsamong them. Importing it is neither a cycle nor a cost — by the time any resolution step runs, the
module is loaded. Ninety-three
from ...overrides importstatements were nonetheless writteninside the functions that use them, which reads as though laziness were required when mostly it
was habit.
Eight of the twenty files this touches had the lazy form before this series, so it is not only a
cleanup after it.
Modifications
The statements that buy nothing become module-scope imports.
Twenty-three more were mixed: a name the tests patch, plus names riding along on the same
statement that nothing patches. Splitting those frees thirty-six more riders. The
_dsa_*passcallables in
model_hookare the shape of it —— three names, one reason. The two passes are handed to
run_post_process_passand never lookedup through the module, so they were lazy only by association. The seam stays a call-site import;
they go to the top.
model_hookandmoe_hookare the exception: their import block also carries the two propertiesPR 2 lifted, so the two changes cannot be separated by hunk without an intermediate commit that
does not compile. Their split lands in PR 2 with the move it is entangled with, and the counts
below are for the whole series.
What is left is load-bearing, for exactly two reasons and nothing else. They are worth writing
down because they decide where the next one goes.
The module is a test seam.
use_mla_backend,attention_backends_of,model_config_ofandsupports_mamba_cache_extra_bufferare patched aspatch("...overrides.<name>"). The fourdeclaration paths —
declare_resolution,declare_late_resolution,declare_direct_writes,run_post_process_pass— are wrapped bysetattron the module intest_no_entry_changes_after_it_is_recorded, which watches every declaration a resolutionmakes.
A
from ... importname is a copy. Hoisting these turns one seam into six or twenty: a testthat patches the source module reaches none of the copies, and a test that patches one consumer
reaches only that one. Hoisting the declarers is what this commit tried first, and the census
guard failed exactly as it was designed to — it recorded nothing, because the wrapper it installed
on the module was no longer what the callers called.
Import order.
runtime_contextis imported byutils.common, whichoverridesneeds, so itsits below
overridesand cannot import it at module scope. That one is a cycle, not apreference; it was found by the import smoke check, not by reasoning.
That leaves a rule a reader can check:
overridesimportruntime_context)Accuracy Tests
The 62-shape resolution probe is byte-identical to the base commit. A dummy resolution is
unchanged at ~9 ms and loads the same six
arg_groupsmodules — which is the point: the lazinessthese statements were written for was already provided by the families' own import placement, not
by the
overridesimport.605 registered config tests pass,
test_no_entry_changes_after_it_is_recordedamong them — it isthe guard that fails the moment a declarer is hoisted.
Two properties worth stating because a hoist can break them silently: every newly module-bound name
is actually used in its file (no unused hoist), and no function still imports locally a name the
same file now binds at module scope (no
UnboundLocalfrom late shadowing). Both hold by AST overthe commit. The newly eager modules are also off the
overrides→utils.common→runtime_contextpath, which is the cycle the keep-list's second reason protects.Speed Tests and Profiling
None beyond the dummy-resolution timing above.
Checklist
Review and Merge Process
Mechanical apart from the keep-list, and the table above is the whole of it: eight names stay lazy
because the module is where tests patch or watch them,
runtime_contextstays lazy because ofimport order, and nothing else stays lazy at all. A reviewer can check the claim by re-running the
same scan.
CI States
Latest PR Test (Base): 🚫 Run #33249957988
Latest PR Test (Extra): 🚫 Run #33249957857
Latest PR Test (AMD ROCm 7.2): 🚫 Run #33249958152