Repository navigation
Conversation
Review (independent; COMMENT only, not an approval)Reviewed: new PR, full diff Thanks for scoping this to two skills and adding retrieval tests. Several items need fixing or clarification before this can pass. Blocking
Non-blocking
Checked
Not verified: upstream vllm-ascend source at either pin (only the in-repo source-notes), and any CI result. |
1fb3a4b to
870c5a7
Compare
Skill evaluationPLANNED: not executed; there are no task outcomes yet.
Author verification steps (nonpaid; not yet run)
python tools/build_skill_index.py --source-revision 79a60141087458919ef32a629ba5ae4f41f708d6 --checkThe declared source pin matches this head's canonical library bytes. The command has not run: retain its exit code and full drift/success output. It checks the generated projection, not claim truth or skill efficacy. Evidence and logs · Retention: 30 days. Plan bindings, budget, frozen exam and advisory detailsAdvisory: obtain missing evidenceRetained evidence availability: planned / unexecuted. No approval or merge authorization is issued.
Model/settings:
Automatic execution / cumulative PR budget: {"reason":"Trusted configuration or pricing missing, invalid or expired","remaining_usd":"unknown","reserved_or_unresolved_usd":"unknown","spent_or_legacy_usd":"unknown","state":"blocked"}Projection check selection (not an executed result): {"declared_in":"src/inferrsi/data/repo_packs/inferrsi/knowledge/README.md","source_revision":"79a60141087458919ef32a629ba5ae4f41f708d6","status":"ready"}Provider/settings: Uncovered / unsupported paths (108)
|
There was a problem hiding this comment.
PR review verdict — DO NOT PASS
Reviewed base 3f560642a44a1496aa4984ab936517693be2c788 and head 870c5a764d43a81e1caeeba096fedaf7d69f1a44. Full diff (diffMode: full, previousHead unset). No prior review body was in seed, so nothing earlier is marked resolved. Dong-bot, independent of author Rohank143.
This review does not approve and does not change the #350 scope. I will re-read after the author pushes.
Blockers
-
Packaged ACLGraph advice contradicts the procedure and mis-states the capture key. Severity: blocking. The catalog will emit the wrong contract.
src/inferrsi/data/skill_library/hardware/ascend/ascend-aclgraph-capture-replay/metadata.jsonapplicability.scope (adds "dynamic shape bucketing") and the new recommendations entry: "Enforce dynamic shape bucketing and batch size padding to match cudagraph_capture_sizes; avoid unbucketed variable sequence lengths triggering repeated graph captures."src/inferrsi/data/skill_library/hardware/ascend/ascend-aclgraph-capture-replay/SKILL.mdprocedure step 3 ("Evaluate shape and batch bucketing conditionally") says that same padding is a conditional suggestion that depends on the upstream vLLM ModelRunner/scheduler contract, and that it is an internal wrapper guarantee only in the negative: the paragraph says it is not a wrapper guarantee. The same step says a mismatch is a graph-cache miss or an eager fallback. The recommendation says "Enforce" and "repeated graph captures."docs/skill-coverage.mdnow publishes that scope and "4 advice", so retrieval follows the recommendation string.- Failure path: a caller pads sequence length to the power-of-two list and treats recapture as the failure. Step 3's own list (
cudagraph_capture_sizes/BatchDescriptor, examples 1, 2, 4, 8, 16, 32, 64) is a discrete batch key. - Expected correction: one wording in
metadata.jsonand step 3. State that the pinned wrapper keysBatchDescriptor/ capture size. Keep any padding on the upstream runner/scheduler side, and keep it conditional. Cite a new source-notes fact for that key. The label[F1–F2]still points at execution-path and dispatch classification; this diff does not extendreferences/source-notes.mdwith a batch-descriptor fact.
-
New FULL-path stream order is not shown against the pin. Severity: blocking missing evidence for a control-flow claim.
SKILL.mdprocedure step 5 states as fact: ENPU enabled →update_stream.synchronize()beforereplay; ENPU disabled →replaythenupdate;FULLsynchronizes the current stream when ENPU is disabled, with a draft-model-plus-EAGLE exception;weak_ref_outputonly on the last piecewise graph; capture returns localoutput, replay returnsentry.output. It cites[F4–F5]and tells the reader to follow source-notes.- This diff does not change
ascend-aclgraph-capture-replay/references/source-notes.md.tests/test_vllm_ascend_distill_skills.pyonly checks that the pin8a8e1e05ecb64a30804c99c8f362909f9a0cb5bf, the substringacl_graph.py,cudagraph_capture_sizes, and the substringENPUoccur. - Failure path: if
ENPUis not the condition invllm_ascend/compilation/acl_graph.pyat that pin, the procedure teaches the wrong synchronize/replay order. - Expected correction: record the pinned condition in source-notes (real symbol, both orders, EAGLE exception, weak-output and return-value rules) and use that same symbol in step 5. Remove any branch the pin does not show.
Non-blocking
- Compiler Passes bullet in the same SKILL.md spells TorchAir as "Torchair".
- Step 6 no longer says
FULL/FULL_DECODE_ONLYis only for mostly uniform decode, and it no longer says that choice can change compilation behavior. Step 1 still says context-parallelFULLis insufficiently supported. - Tag
batch-1on the ACLGraph skill is a delivery label. - The new test does not check that the recommendation and step 3 agree, or that the stream-order symbol matches source-notes.
Verified in this diff
- Both skills stay
candidate/not_run. No E2E or speedup claim was added. - Model adaptation writes the pin out to
bc8115ed7fdc7e500320b5de70742ef049b4c1a2, addsreferences/source-notes.md, splits the FP8 scale step into a source fact versus a reciprocal-scale interpretation, and addsrefusal_pointplusprovenance_readsets: ["references/source-notes.md"]. The coverage row matches that shape (refusal_point, 3 advice, 1 readset). - ACLGraph coverage row matches the edited metadata shape (bucketing scope, 4 advice, 1 readset). Routing text points at model-adaptation,
ascend-graph-mode, andhost-memory-management.
Not verified here
tests/test_vllm_ascend_distill_skills.pywas not executed.python tools/build_skill_coverage.py --checkwas not run; the newdocs/skill-coverage.mddigest was not recomputed.- Pinned
acl_graph.pyat8a8e1e05ecb64a30804c99c8f362909f9a0cb5bfwas not opened. CI was not inspected.
870c5a7 to
b205b12
Compare
|
Thanks for the thorough independent review. All blocking and non-blocking points have been addressed in the latest push: Blocking Items Resolved:
Non-blocking Items Resolved:
Verification Run:
|
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base 3f560642a44a1496aa4984ab936517693be2c788 and head b205b12ce7ca286c0be11f8bc21639c56c4e09cf.
Incremental range 870c5a764d43a81e1caeeba096fedaf7d69f1a44..b205b12ce7ca286c0be11f8bc21639c56c4e09cf. Dong-bot, independent of author Rohank143.
Earlier ACLGraph capture-key finding is resolved on this head. The procedure, metadata.json, packaged ascend-aclgraph-capture-replay.json, and docs/skill-coverage.md (5 advice) state one contract: captured graphs are indexed by BatchDescriptor; request bucketing and padding stay in the caller/runner; confirmed stream-resource exhaustion (insufficient_stream_resources, stream resources are insufficient, or code 207008 with stream resource) is handled by reducing cudagraph_capture_sizes or max_cudagraph_capture_size. The prior review seed ended mid-sentence after that finding; no further blocker text was in seed.
Verified
- ACLGraph scope, refusal point, and five recommendations match across
src/inferrsi/data/skill_library/hardware/ascend/ascend-aclgraph-capture-replay/metadata.jsonandsrc/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-aclgraph-capture-replay.json. - Model-adaptation adds
references/source-notes.md,provenance_readsets, andrefusal_point. The packaged JSON and coverage row (refusal_point, 1 readset, 3 advice) match. The FP8 scale multiply is labeled an implementation interpretation in bothSKILL.mdand source-notes. Historical prefixbc8115edis recorded asbc8115ed7fdc7e500320b5de70742ef049b4c1a2. - The host-offload link
../../host/host-memory-management/SKILL.mdmatches the existing packaged skill path. tests/test_vllm_ascend_distill_skills.pylocks discoverability for the two skill ids and the new pin, ENPU, and three-tier strings.- Knowledge records retarget the snapshot to
b887480e6c393a2e0a1efb0ee5d2b7ddf57bb10a, with updated content hashes for the two edited skills.
Not verified here
pytest,python tools/build_skill_index.py --source-revision b887480e6c393a2e0a1efb0ee5d2b7ddf57bb10a --check, andpython tools/build_skill_coverage.py --checkwere not run. CI was not inspected.- ACLGraph
references/source-notes.mdis unchanged (6ffbfb68cc7e04923b4fbca2a73a7555b7c3b45a6f40819bb5f3213ef05cf88d). The new ENPUupdate/replayorder andweak_ref_outputsentences were not re-read against F4–F5. - The full SHA for
bc8115edwas not resolved againstvllm-project/vllm-ascend.
Optional follow-ups
- In ACLGraph
SKILL.md, the Compiler Passes bullet still ends with "this skill adds the independently pinned vLLM-Ascend runtime wrapper" underascend-graph-mode. - The
FULL/FULL_DECODE_ONLYproposal dropped the earlier narrowing to uniform decode only, the platform check, and the compilation-behavior side effect. - The three-tier note labels both Tier 2 and Tier 3 as
hardware/ascend/.
Re-review (independent; COMMENT only, not an approval)Reviewed range: previous head Earlier findings: status
Minor (non-blocking)
NextOnce a maintainer approves the workflow run and |
Incremental review (COMMENT only, not an approval)Reviewed range: Blocking
The other points from my previous comment (6761392302) still stand. |
Incremental review (COMMENT only, not an approval)Reviewed range: Resolved
Still blocking
The minor points from comment 6761392302 still stand. |
Incremental review (COMMENT only, not an approval)Reviewed range: Resolved
Still blocking
Suggestion: before pushing, run all three checks locally together ( |
|
Fixed the review blockers in the latest commits. What changed:
Verification:
|
|
Updated the knowledge-model-policy declaration to the current PR head |
Incremental review (COMMENT only, not an approval)Reviewed range: Resolved
Still blocking
Minor (non-blocking, still open from 6761392302)
If you address the minor points, please regenerate the index in the same push and run |
|
Addressed the remaining review follow-ups.
Verification:
The remaining merge blocker is maintainer approval for the fork workflow/full CI. |
Incremental review (COMMENT only, not an approval)Reviewed range: Resolved
Still blocking (unchanged)
Nit (non-blocking)
|
Refs #350
Problem and scoped delivery slice (Batch 1)
This pull request implements the maintainer-assigned Batch 1 knowledge distillation slice for vLLM Ascend knowledge refinement under #350, adhering strictly to
docs/knowledge-base.mdanddocs/knowledge-model-policy.md.In scope for this slice:
ACLGraph Capture/Replay Refinement (
hardware/ascend/ascend-aclgraph-capture-replay):BatchDescriptorindexing,NONE/mismatch direct runnable bypass,entry.aclgraph is Nonetriggering capture aftervalidate_cudagraph_capturing_enabled(), and discrete capture sets bounded bycudagraph_capture_sizes/max_cudagraph_capture_size).kwargsand nested containers are not checked).FULLgraph mode whereuse_updatable_graph(attn_backend)holds (ENPU:updatethenreplay, noting caller model runner assignment; non-ENPU:replaythenupdate).torchnpu-device-memory-lifecycle("at another pin; not evidence that this wrapper improved performance").alloc sq cq fail, error207008), intermediate tensor out-of-bounds, and non-contiguous stride graph capture crashes.Model Adaptation Refinement (
hardware/ascend/ascend-model-adaptation):framework.\nwithout trailing whitespace.<name>.weightand<name>.weight_scale_invtensors as the upstream source fact, while designating the arithmetic multiplication formula as an operational implementation interpretation for reciprocal scales. Clarified that scale granularity and broadcasting/expansion for block-quantized checkpoints are checkpoint-specific.tp_size > num_kv_heads.references/source-notes.mdconfirming full commit SHAbc8115ed7fdc7e500320b5de70742ef049b4c1a2resolves the historical prefixbc8115edagainstvllm-project/vllm-ascend.Metadata, Coverage, Index & Retrieval:
batch-1delivery labels from metadata tags.metadata.jsonfor both skills with explicit refusal points, applicability scopes, input contracts, and provenance readset pointers.host-memory-managementas an existing cross-reference and keptascend-operator-tuningoutside this slice (candidate/readset review needed).docs/skill-coverage.mdviapython tools/build_skill_coverage.py(verified with--check).python tools/build_skill_index.py --source-revision 79a60141087458919ef32a629ba5ae4f41f708d6(verified with--check).tests/test_vllm_ascend_distill_skills.pycovering catalog token discoverability viasearch_knowledge, candidate/not_run metadata contracts, and instruction content.Out of scope:
ascend-operator-tuningremains an existing candidate awaiting separate review).Knowledge Authorship Declaration
Evidence & Verification Run
python -m pytest tests/test_vllm_ascend_distill_skills.py tests/test_knowledge_contract.py-> 20 passed in 8.30spython -m pytest tests/test_skill_library.py-> 10 passed in 16.86spython tools/build_skill_coverage.py --check-> docs/skill-coverage.md matches its sources (exit code 0)python tools/build_skill_index.py --source-revision 79a60141087458919ef32a629ba5ae4f41f708d6 --check-> Skill index is current: 80 metadata records (exit code 0)python -m ruff check tests/test_vllm_ascend_distill_skills.py-> All checks passed!Proposed Retrieval Test Cases (#255 Evaluation Protocol)
"ACLGraph capture replay"ascend-aclgraph-capture-replayascend-graph-mode."FP8 checkpoint"ascend-model-adaptation"KV replication TP"ascend-model-adaptation