Skip to content

Add more unit tests to CI - #880

Merged
Ying1123 merged 1 commit into
mainfrom
more-test
Aug 2, 2024
Merged

Ying1123 merged 1 commit into
mainfrom
more-test

Conversation

@Ying1123

@Ying1123 Ying1123 commented Aug 2, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@Ying1123
Ying1123 merged commit 76e5908 into main Aug 2, 2024
@Ying1123
Ying1123 deleted the more-test branch August 2, 2024 01:14
timethink pushed a commit to timethink/sglang that referenced this pull request Mar 9, 2025
hhhh1252023 pushed a commit to hhhh1252023/sglang_public that referenced this pull request Jun 27, 2026
Automated code for submitting mimo_v2_flash model requests
efschu pushed a commit to efschu/htsglang that referenced this pull request Aug 26, 2026
…f from a list

THE QUESTION WAS ASKED OF MY OWN EARLIER WORK, AND THE HONEST ANSWER WAS NO.

Three duck-typed attribute misses on the Unified family had been found one at a
time -- sgl-project#832 `evictable_size` (killed all three ranks on W29), sgl-project#872
`_drain_storage_control_queues_local`, #872b `value`/`*lock_ref` on
UnifiedTreeNode. All three were INSTANCE fixes. The two conformance suites from
sgl-project#872/#872b were the first class-level checks in this tree, and measuring them
shows they were not enough:

  * the sgl-project#872 suite reads probed names out of the SOURCE -- genuinely derived,
    but for ONE module (`hicache_flip_writeback`);
  * the #872b suite carries `expected_unreadable = {"UnifiedTreeNode"}` and
    pins `value` plus the `*lock_ref` trio. That is a LIST. Measured:
    `TreeNode` exposes SIX names `UnifiedTreeNode` lacks -- value, lock_ref,
    host_value, host_ref_counter, protect_host, release_host. The list covered
    TWO of six: exactly the ones that had already hurt.

So the #872b half was a third instance fix in class clothing, and the fourth
instance would have landed. A list extended at each new finding is a COUNTER.

WHAT THIS IS INSTEAD. The surface is computed from the BASE at test time:
whatever the non-Unified counterpart provides and the Unified one does not.
Consumer modules are then scanned for duck-typed reads of those names. Nothing
about the three known pains is privileged, and a new divergence enters the
surface without anyone editing this file. Measured on `UnifiedRadixCache`: 17
names `RadixCache` provides that it lacks, 40 against `MambaRadixCache`.

THE DISCRIMINATOR, WITHOUT WHICH IT CRIES WOLF. Not every `hasattr` on a
diverging name is a cross-class probe. `session_radix_cache` guards
`_session_leaves` with `hasattr` and then CREATES it
(`_ensure_session_radix_state`) -- lazy init, entirely correct, and a false
positive here. The rule that separates them, and it is derived rather than
listed: a probe is a cross-class miss only if the probing module NEVER ASSIGNS
the attribute itself. Without it this gate fires on working code, and a gate
that fires on working code gets muted -- which is how the original condition
survived three findings.

WHAT IT FOUND THAT NO INSTANCE FIX EVER TOUCHED. Four sites are derived today.
Three were known. The fourth is `UnifiedTreeNode.host_value`, duck-typed in
`hicache_demotion.is_persistable` -- named as a landmine during #872b but
covered by NO check until now. It remains UNREACHABLE (hicache_demotion is
imported only by hiradix_cache.py, zero references from unified_radix_cache),
and it is RECORDED rather than fixed, so it cannot become live unnoticed.
`sgl-project#832`'s `evictable_size` correctly produces NO finding -- its consumer now
reads `component_evictable_size_`, so the check confirms that fix still holds.

THE NAMING TRAP, RECORDED because it is why two readers reached the same wrong
conclusion independently: `Unified*` spans TWO unrelated subsystems.
`UnifiedRadixCache`/`UnifiedTreeNode` are the radix TREE and are LIVE on this
rig. The unified memory POOLS are gated behind `--enable-unified-memory`, which
is False by default (server_args.py:1378), is set nowhere in the tree, and whose
handler asserts `speculative_algorithm is None` while this rig always runs MTP.
A shared prefix suggested a shared gate that does not exist, so this suite pairs
only the TREE classes and says why.

TESTS (hermetic; no boot; no card touched). CUDA_VISIBLE_DEVICES="" verified at
the process, `nvidia-smi --query-compute-apps` empty throughout, cards 0/0/0.

* CAN-FAIL ON A NAME THAT NEVER HURT, which is the point of the exercise:
  - un-recording `host_value` (no instance fix ever touched it) -> gate red;
  - injecting a BRAND-NEW divergent probe into real consumer code
    (`getattr(node, "protect_host", ...)` in hicache_demotion -- a name in no
    list anywhere) -> gate red AND names it in the failure message;
  - dropping the lazy-init discriminator -> 2 red, incl. the `_session_leaves`
    false positive returning.
  Green restored after each. If only the three known names had reddened, the
  answer to "does this cover the family" would have been no.
* partitioned tier-2 gate: wide 3775 / narrow 291 / serial 353 passed, 24
  exclusions reported, names agrees with the summary on every lane. TWO
  failures, neither mine -- this commit adds a single test file under
  mem_cache/ and touches no product code:
    - test_pp_admission_wraparound_never_blocks: PRE-EXISTING, proved earlier
      on the clean base 726f11e where that module fails TWO tests, i.e.
      MORE than here;
    - test_pp_admission_chain_flush_deadlock_795: PASSES in isolation on this
      tree (both modules together: 1 failed, 6 passed), so it is the
      order/contention shape the narrow lane's bounded worker count exists to
      avoid, already seen twice this session.
* test/registered/unit/mem_cache: 2 failed / 1814 passed -- the known
  test_acceptance_emitters_758 RefillTiming pair; +6 over 1808 is this suite.

WHERE THIS STRUCTURALLY DOES NOT REACH, named in the module rather than left to
be discovered: factory indirection (a class produced by a builder, never named
here); `getattr(obj, some_variable)` where the name is computed; registry or
dispatch-table lookup, where neither side appears in the source; and a consumer
that reads `obj.attr` directly and lets AttributeError fly -- out of scope by
intent, because that failure is LOUD and therefore a different, better problem.
The same methodological limit as the post-set enumeration and the sgl-project#880
reachability sweep: stated, not papered over.

NOT ESTABLISHED. Whether the pairing itself is complete beyond the radix-tree
module -- `test_every_unified_class_is_paired` enforces it for
`unified_radix_cache`, but a Unified* class living in another module is outside
that scan by construction.
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.

1 participant