Skip to content

feat: refine vLLM Ascend ACLGraph and model adaptation knowledge (Refs #350) - #382

Open
Rohank143 wants to merge 9 commits into
InferMatrixLab:mainfrom
Rohank143:feat/issue-350-vllm-ascend-batch1
Open

Rohank143 wants to merge 9 commits into
InferMatrixLab:mainfrom
Rohank143:feat/issue-350-vllm-ascend-batch1

Conversation

@Rohank143

@Rohank143 Rohank143 commented Nov 10, 2026 •

Copy link
Copy Markdown

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.md and docs/knowledge-model-policy.md.

In scope for this slice:

  1. ACLGraph Capture/Replay Refinement (hardware/ascend/ascend-aclgraph-capture-replay):

    • Grounded batch descriptor matching and capture sizing strictly in F1, F2, and F6 (BatchDescriptor indexing, NONE/mismatch direct runnable bypass, entry.aclgraph is None triggering capture after validate_cudagraph_capturing_enabled(), and discrete capture sets bounded by cudagraph_capture_sizes / max_cudagraph_capture_size).
    • Retained the caveat that equal positional tensor addresses establish neither updated contents nor semantic correctness (kwargs and nested containers are not checked).
    • Synchronized multi-stream execution rules explicitly qualified to the update-aware FULL graph mode where use_updatable_graph(attn_backend) holds (ENPU: update then replay, noting caller model runner assignment; non-ENPU: replay then update).
    • Retained explicit separate-pin boundaries for torchnpu-device-memory-lifecycle ("at another pin; not evidence that this wrapper improved performance").
    • Classified exact Ascend failure modes: HDK stream allocation failure (alloc sq cq fail, error 207008), intermediate tensor out-of-bounds, and non-contiguous stride graph capture crashes.
  2. Model Adaptation Refinement (hardware/ascend/ascend-model-adaptation):

    • Documented the three-tier responsibility architecture as an InferRSI routing convention across repository owners: Tier 1 (vLLM generic engine), Tier 2 (vLLM Ascend plugin), Tier 3 (Ascend/CANN/torch_npu bottom layer).
    • Reverted frontmatter formatting so the description cleanly ends in framework.\n without trailing whitespace.
    • Grounded FP8 load-time BF16 dequantization with paired <name>.weight and <name>.weight_scale_inv tensors 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.
    • Documented Tensor Parallelism (TP) Grouped-Query Attention (GQA) key-value head replication when tp_size > num_kv_heads.
    • Detailed multimodal input validation gates with strict fail-fast staging.
    • Added bundled references/source-notes.md confirming full commit SHA bc8115ed7fdc7e500320b5de70742ef049b4c1a2 resolves the historical prefix bc8115ed against vllm-project/vllm-ascend.
  3. Metadata, Coverage, Index & Retrieval:

    • Removed temporary batch-1 delivery labels from metadata tags.
    • Updated owner-local metadata.json for both skills with explicit refusal points, applicability scopes, input contracts, and provenance readset pointers.
    • Preserved host-memory-management as an existing cross-reference and kept ascend-operator-tuning outside this slice (candidate/readset review needed).
    • Regenerated docs/skill-coverage.md via python tools/build_skill_coverage.py (verified with --check).
    • Regenerated all 80 package knowledge records via python tools/build_skill_index.py --source-revision 79a60141087458919ef32a629ba5ae4f41f708d6 (verified with --check).
    • Added 7 automated tests in tests/test_vllm_ascend_distill_skills.py covering catalog token discoverability via search_knowledge, candidate/not_run metadata contracts, and instruction content.
    • Documented proposed retrieval test cases aligned with the [Tracking] Skill Indexer: retrieval efficiency and accuracy #255 evaluation protocol.

Out of scope:

  • Full library re-organization or directory restructuring.
  • Operator kernel tuning (ascend-operator-tuning remains an existing candidate awaiting separate review).
  • Modifying upstream vLLM Ascend code or running unsupported physical hardware experiments.

Knowledge Authorship Declaration

{
  "schema_version": 1,
  "head_sha": "755e548be8d5b27dc40d7c41735431e55be754fe",
  "human_only": true,
  "material_ai_authors": []
}

Evidence & Verification Run

  • python -m pytest tests/test_vllm_ascend_distill_skills.py tests/test_knowledge_contract.py -> 20 passed in 8.30s
  • python -m pytest tests/test_skill_library.py -> 10 passed in 16.86s
  • python 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)

  1. Query: "ACLGraph capture replay"
    • Expected Primary Hit: ascend-aclgraph-capture-replay
    • Negative Hit: Must not conflate with uncaptured generic ascend-graph-mode.
  2. Query: "FP8 checkpoint"
    • Expected Primary Hit: ascend-model-adaptation
    • Context: Triggers load-time weight dequantization onto Ascend BF16 memory without native FP8 kernel acceleration.
  3. Query: "KV replication TP"
    • Expected Primary Hit: ascend-model-adaptation
    • Context: Resolves GQA head count starvation when tensor parallel world size exceeds KV head count.

Copy link
Copy Markdown
Collaborator

Review (independent; COMMENT only, not an approval)

Reviewed: new PR, full diff b8df3715709b806d06c4bfc9158ab41b18ded11e .. 1fb3a4b5e9bb7395ef0ce70dd4407eea05563a3f (1 commit, 8 files). Refs #350.

Thanks for scoping this to two skills and adding retrieval tests. Several items need fixing or clarification before this can pass.

Blocking

  1. Generated knowledge records not regenerated. Both skills' SKILL.md and metadata.json changed: tags (vllm-ascend, batch-1, stream-synchronization), scope, the refusal_point and the new references/source-notes.md readset. The frontmatter description also changed (see 5). However, src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-model-adaptation.json and ascend-aclgraph-capture-replay.json are untouched. On base they still carry the old summary, the old tags and source_files hashes without source-notes.md, and they pin d648f257. Please regenerate with python tools/build_skill_index.py --source-revision <a committed revision containing these bytes> and include the --check output. If you believe the records are intentionally left at the old pin, please explain why the shipped index can describe different content.
  2. New procedure claims in ascend-aclgraph-capture-replay/SKILL.md step 3 ("Enforce dynamic shape bucketing") aren't supported by the cited facts. It cites [F1–F2], but the existing references/source-notes.md (unchanged in this PR) doesn't contain the example size list 1, 2, 4, 8, 16, 32, 64. It also doesn't contain the claims that prefill variable sequence lengths cause capture misses or eager fallback, or that padding routes requests to the nearest captured size. F2 actually says that a missing BatchDescriptor entry triggers capture (after validate_cudagraph_capturing_enabled()), not eager fallback, and that NONE/mode mismatch bypasses. Please either add pinned source evidence (file, line range at 8a8e1e05…) to source-notes or drop/condition these statements. The same goes for the new metadata.json recommendation "Enforce dynamic shape bucketing and batch size padding…".
  3. Step 5 adds update_stream.synchronize() before replay for ENPU. F5 only records "ENPU executes update then replay; non-ENPU executes replay then update". It says the ENPU ordering is assigned to the model runner, which was not inspected. Please remove the synchronize() detail or add the inspected source for it.
  4. The three-tier responsibility text in ascend-model-adaptation (SKILL.md new section and references/source-notes.md "Three-Tier Responsibility & Attribution") isn't sourced. The notes say the evidence origin is fp8-on-npu-lessons.md / multimodal-ep-aclgraph-lessons.md at bc8115ed. The tier assignments (PagedAttention/RPC in Tier 1, "native host KV offload descriptor pooling" as a vLLM-Ascend plugin feature, aclmdlExecute/geExecutor/HCCL mesh scratch in Tier 3) are not attributed to either document. Please mark them as an InferRSI routing convention rather than a source fact, or cite where each comes from. Also, the full SHA bc8115ed7fdc7e500320b5de70742ef049b4c1a2 is new; please confirm it was resolved against vllm-project/vllm-ascend rather than expanded by hand.
  5. Frontmatter change in ascend-model-adaptation/SKILL.md lines 3–4. The closing ' moved to column 0. It still parses, but description now ends in "framework. " (trailing space) instead of "framework.\n". That's an unintended content change to a protected path. Please revert those two lines unless the change is deliberate.
  6. No CI has run on this head. list check-runs for 1fb3a4b is empty, so full-check, installed-package, the knowledge-model-policy declarations and SkillEval planning haven't produced results. These are protected paths (src/inferrsi/data/skill_library/**), and the declaration says human_only: true. Please make sure the policy check runs and passes on the head, and that the declaration truthfully covers all authorship.

Non-blocking

  • ascend-aclgraph-capture-replay/SKILL.md step 4: the old text said the address assertion doesn't cover "semantic correctness". The new wording drops that caveat, though F3 says equal addresses don't establish updated contents. Please keep it.
  • The related-capabilities rewrite drops "at another pin" for torchnpu-device-memory-lifecycle and the "not evidence that this wrapper improved performance" framing. The separate-pin boundary is worth keeping.
  • FP8 formula weight_fp8.to(torch.bfloat16) * weight_scale_inv: for block-quantized checkpoints, weight_scale_inv is usually per-block and needs broadcasting or expansion. Does the bc8115ed source specify the scale granularity? If not, please say that the shape handling is checkpoint-specific.
  • Metadata tag batch-1 is a delivery-slice label, not a retrieval concept. Consider dropping it.

Checked

  • F5/F6 classifier wording (_is_stream_resource_capture_error, 207008 + stream resource, alloc sq cq fail, HDK 25.5.1+) matches the existing source-notes.
  • host-memory-management link target exists at hardware/host/host-memory-management/SKILL.md on base.
  • New tests reference strings present at the head (tp_size > num_key_value_heads, weight_scale_inv, --load-format dummy). I have not run them.

Not verified: upstream vllm-ascend source at either pin (only the in-repo source-notes), and any CI result.

@Rohank143
Rohank143 force-pushed the feat/issue-350-vllm-ascend-batch1 branch from 1fb3a4b to 870c5a7 Compare November 10, 2026 06:58
@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

Skill evaluation

PLANNED: not executed; there are no task outcomes yet.
No approval, model efficacy or current-head test result is implied.

  • Next: run the pinned source/index --check below at head 755e548be8d5b27dc40d7c41735431e55be754fe.
  • Task selection: no supported automatic comparison; choose a relevant source/content check and disposition uncovered claims.
  • Selected comparisons: 0 · Estimated task calls: 0. Uncovered/unsupported changes: 108; Canonical library paths: 5; Generated projection paths: 81; Other paths: 22. These are paths, not independent claims.
  • Return: command, input/source pin, exit status, outputs/failures and untested claims for independent verified/refuted/unverified disposition.
Author verification steps (nonpaid; not yet run)
  1. Bind the checkout. Run git rev-parse HEAD; it must equal 755e548be8d5b27dc40d7c41735431e55be754fe. If it differs, obtain a current plan; do not reuse old results.
  2. Check source/index consistency. At that PR checkout, using the reviewed repository tool and its documented development dependencies, run:
python tools/build_skill_index.py --source-revision 79a60141087458919ef32a629ba5ae4f41f708d6 --check

The 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.
No supported automatic comparison is selected. Review the canonical changes and explain which claims need evidence, are unaffected, or remain unverified; do not treat generated records as separate behavioral claims.
Uncovered scope by location: Canonical library paths: 5; Generated projection paths: 81; Other paths: 22. Path counts are not claim counts; no metadata-only conclusion is inferred.
3. Return evidence for independent review: head, check/task/source pin, command, environment, exit status, outputs/failures and untested claims. Label scripted controls. Request each claim's disposition: verified, refuted or unverified; this plan proves neither model provenance nor benefit.
Check recipes and evidence limits

Evidence and logs · Retention: 30 days.
Retained artifacts are verifiable only before Unix time 1796903276 while the linked artifact is available. Expired evidence is unverified.

Plan bindings, budget, frozen exam and advisory details

Advisory: obtain missing evidence

Retained evidence availability: planned / unexecuted. No approval or merge authorization is issued.
Applicable only while these revisions and inputs match the current PR.
Paid execution requires trusted label authorization and cumulative PR reservation. Unknown accounting blocks new paid calls; planning is not execution.
Pipeline evidence only; offline fixed outputs do not establish skill efficacy.
Unchanged-correct is valid neutral evidence, not incremental benefit.
Content/source review and authorship admission remain separate.
Trusted regrading checks retained answers, not model/hardware provenance or a hostile executor; this is not an efficacy certification.

  • Retained evidence is planned / unexecuted.
  • No supported comparison cases were selected.
  • Uncovered/unsupported scope needs a case or explicit reviewer disposition.
    Plan SHA-256: 0194ed925486546807d8ce64d7129d4548cf19fa672928cb99c193358b8c889b
    Base: 3f560642a44a1496aa4984ab936517693be2c788 · Head: 755e548be8d5b27dc40d7c41735431e55be754fe
    Trusted implementation: bd61d20c52b4d772a3ea282ae9cb3c0fd50075df

Model/settings: {"effort":"not_applicable","name":"claude-opus-5-5"}
Budget: {"comparisons":0,"estimated_calls":0,"limits_per_comparison":{"campaign_seconds":600,"max_task_calls":2,"repeats":1,"timeout_seconds":180,"token_stop_new_trials":200000},"provider_config_sha256":"b28d80f8194fbe0f5a9f01515efb24b218979fd45f702776560da276f4b30d04","reservation":null}

Skill Comparison Cases Estimated calls

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: {"client":{"api_version":"2023-06-01","endpoint":"https://yibuapi.com/v1/messages","kind":"anthropic-messages-full-context-v1","max_output_tokens":4096,"max_read_bytes":32768,"max_request_bytes":262144,"max_requests":1,"max_tool_calls":12,"provider":"yibuapi"},"limits":{"token_stop_new_trials":200000},"model":{"effort":"not_applicable","name":"claude-opus-5-5"},"pricing":{"currency":"USD","expires_at":1793271228,"group":"default","max_input_tokens":1000000,"model":"claude-opus-5-5","rate_evidence":"https://github.com/InferMatrixLab/InferRSI/issues/215#issuecomment-6467531341","rates":{"cache_read":"0.4","cache_write":"10","input":"8","output":"40"},"request_fee":"0","unit":"per_million_tokens"},"spend_limit":{"amount":"200","currency":"USD"}}

Uncovered / unsupported paths (108)
  • uncovered: ".github/workflows/skill-eval-pr.yml"
  • uncovered: "docs/examples/skill-comparison/addition.json"
  • uncovered: "docs/examples/skill-comparison/cases.json"
  • uncovered: "docs/examples/skill-comparison/pr133-diagnostic-case05.json"
  • uncovered: "docs/examples/skill-comparison/report-example.json"
  • uncovered: "docs/examples/skill-comparison/report-example.md"
  • uncovered: "docs/examples/skill-comparison/update.json"
  • uncovered: "docs/examples/skill-eval-pr/summary-example.md"
  • uncovered: "docs/module-layout.md"
  • uncovered: "docs/skill-coverage.md"
  • uncovered: "docs/skill-eval-comparison.md"
  • uncovered: "docs/skill-eval-pr.md"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/README.md"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-aclgraph-capture-replay.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-device-health.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-distributed-parallel.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-graph-mode.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-model-adaptation.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-operator-profiling.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-operator-tuning.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-optimization-casebook.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-performance-analysis.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-video-diffusion-optimization.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascendc-tiling-design.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/cann-attribution-overlap.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/torch-npu.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/torchnpu-device-memory-lifecycle.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/torchnpu-pinned-host-memory-lifecycle.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/vllm-ascend.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/assess-serving-preemption.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/assess-split-backward-schedule.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-fixed-workload-claim.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-h3-metal-fasth3-semantics.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-h3-runtime-residency.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-llama-runtime-state.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-recurrent-kv-finalization.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-sd-cpp-residency.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-sglang-diffusion-lifetimes.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/audit-vllm-kv-lifetimes.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/bound-windowed-attention-replay.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/budget-speculative-verification.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/calibrate-serving-simulation.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/cgroup-resource-controls.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/checkpoint-recovery-consistency.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/choose-parallel-training-plan.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/cite-check-downstream-pr.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/claim-contract.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/comfy-ui.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/competitor-scan.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/dcp-async-save-performance.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/dense-performance-evidence.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/diagnose-vllm-admission-pressure.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/diffusers-invocation-state.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/draft-acceptance-issue.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/dwarfstar.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/evaluate-elastic-serving-placement.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/generation-cache-lifecycle.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/host-memory-management.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/host-overhead-optimization.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/husky.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/inference-benchmark-design.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/inference-optimization-casebook.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/kernel-writing-guide.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/kv-tier-management.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/lazy-evaluation-stream-lifetimes.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/lily.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/lithos-metal.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/local-dream.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/mistral-rs.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/mlc-llm.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/mlx-swift-lm.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/npu-skill-factory.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/optimization-search-policy.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/os-process-placement.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/performance-ceiling-analysis.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/plan-activation-residency.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/project-maya.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/pytorch.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/repository-evolution-analysis.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/rsi-evidence.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/skill-evolution-playbook.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/splash.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/storage-io-measurement.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/training-input-pipeline.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/training-resource-budget.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/video-diffusion-optimization.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/vllm-metal.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/common/skills/weight-loading-optimization.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/cuda/skills/audit-afd-p2p-exchanges.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/cuda/skills/audit-flare-session-lifecycle.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/cuda/skills/diagnose-attention-bottlenecks.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/cuda/skills/host-overhead-profiling.json"
  • uncovered: "src/inferrsi/data/repo_packs/inferrsi/knowledge/cuda/skills/reason-hopper-kernels.json"
  • uncovered: "src/inferrsi/data/skill_library/hardware/ascend/ascend-aclgraph-capture-replay/SKILL.md"
  • uncovered: "src/inferrsi/data/skill_library/hardware/ascend/ascend-aclgraph-capture-replay/metadata.json"
  • uncovered: "src/inferrsi/data/skill_library/hardware/ascend/ascend-model-adaptation/SKILL.md"
  • uncovered: "src/inferrsi/data/skill_library/hardware/ascend/ascend-model-adaptation/metadata.json"
  • uncovered: "src/inferrsi/data/skill_library/hardware/ascend/ascend-model-adaptation/references/source-notes.md"
  • uncovered: "tests/fixtures/skill-comparison/addition.json"
  • uncovered: "tests/fixtures/skill-comparison/cases.json"
    Additional paths omitted; see retained evidence.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.json applicability.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.md procedure 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.md now 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.json and step 3. State that the pinned wrapper keys BatchDescriptor / 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 extend references/source-notes.md with a batch-descriptor fact.
  2. New FULL-path stream order is not shown against the pin. Severity: blocking missing evidence for a control-flow claim.

    • SKILL.md procedure step 5 states as fact: ENPU enabled → update_stream.synchronize() before replay; ENPU disabled → replay then update; FULL synchronizes the current stream when ENPU is disabled, with a draft-model-plus-EAGLE exception; weak_ref_output only on the last piecewise graph; capture returns local output, replay returns entry.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.py only checks that the pin 8a8e1e05ecb64a30804c99c8f362909f9a0cb5bf, the substring acl_graph.py, cudagraph_capture_sizes, and the substring ENPU occur.
    • Failure path: if ENPU is not the condition in vllm_ascend/compilation/acl_graph.py at 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_ONLY is only for mostly uniform decode, and it no longer says that choice can change compilation behavior. Step 1 still says context-parallel FULL is insufficiently supported.
  • Tag batch-1 on 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, adds references/source-notes.md, splits the FP8 scale step into a source fact versus a reciprocal-scale interpretation, and adds refusal_point plus provenance_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, and host-memory-management.

Not verified here

  • tests/test_vllm_ascend_distill_skills.py was not executed.
  • python tools/build_skill_coverage.py --check was not run; the new docs/skill-coverage.md digest was not recomputed.
  • Pinned acl_graph.py at 8a8e1e05ecb64a30804c99c8f362909f9a0cb5bf was not opened. CI was not inspected.

@Rohank143
Rohank143 force-pushed the feat/issue-350-vllm-ascend-batch1 branch from 870c5a7 to b205b12 Compare November 10, 2026 07:07
@Rohank143

Copy link
Copy Markdown
Author

Thanks for the thorough independent review. All blocking and non-blocking points have been addressed in the latest push:

Blocking Items Resolved:

  1. Regenerated Knowledge Records:
    • Regenerated all 80 package knowledge records via python tools/build_skill_index.py --source-revision b887480e6c393a2e0a1efb0ee5d2b7ddf57bb10a and verified with --check.
    • ascend-model-adaptation.json now includes references/source-notes.md in provenance_readsets and source_files, updated tags, and refusal point.
    • ascend-aclgraph-capture-replay.json now includes stream-synchronization tag, updated scope with batch descriptor matching, and refusal point.
  2. Dynamic Shape Bucketing Grounding:
    • Removed ungrounded example size list (1, 2, 4, 8, 16, 32, 64) and claims about prefill variable sequence lengths / eager fallback.
    • Grounded Step 3 strictly in F1, F2, and F6 (BatchDescriptor indexing, validate_cudagraph_capturing_enabled(), and cudagraph_capture_sizes/max_cudagraph_capture_size resource mitigation).
    • In ascend-aclgraph-capture-replay/metadata.json, replaced the recommendation with: "Verify caller BatchDescriptor stability and dispatch mode before investigating graph replay issues." and "For stream-resource exhaustion, reduce cudagraph_capture_sizes or max_cudagraph_capture_size."
  3. ENPU Synchronization Qualification:
    • Removed .synchronize() detail. Stated simply: on the update-aware FULL path when ENPU is enabled, parameter update executes before graph replay (update then replay), noting that this ordering is assigned to the caller model runner.
  4. Three-Tier Responsibility Sourcing:
    • Explicitly framed the Three-Tier Responsibility model as an InferRSI routing convention across repository owners rather than an upstream source fact.
    • In source-notes.md, confirmed that full SHA bc8115ed7fdc7e500320b5de70742ef049b4c1a2 resolves the historical prefix bc8115ed recorded in src/inferrsi/data/skill_library/README.md#vllm-ascend-version-and-responsibility-routing against upstream vllm-project/vllm-ascend.
  5. Frontmatter Invariant:
    • Restored lines 3–4 of ascend-model-adaptation/SKILL.md so the description cleanly ends with framework.\n without trailing whitespace differences.
  6. CI & Policy Declaration:
    • Pushed head b205b12ce7ca286c0be11f8bc21639c56c4e09cf and updated PR body declaration block.
    • knowledge-model-policy workflow ran and returned state: success (Green).

Non-blocking Items Resolved:

  • Semantic Correctness Caveat: Re-added the caveat in ascend-aclgraph-capture-replay/SKILL.md step 4 that equal addresses establish neither updated contents nor semantic correctness.
  • Related Capabilities Pin Boundary: Restored the "at another pin; not evidence that this wrapper improved performance" framing for torchnpu-device-memory-lifecycle.
  • FP8 Scale Granularity: Clarified that upstream notes require load-time BF16 dequantization using paired scale keys, but do not specify a single universal scale granularity (broadcasting/expansion for block-quantized checkpoints is checkpoint-specific).
  • Metadata Tag Cleanliness: Removed temporary batch-1 delivery labels from metadata tags.

Verification Run:

  • python tools/build_skill_index.py --source-revision b887480e6c393a2e0a1efb0ee5d2b7ddf57bb10a --check -> Skill index is current: 80 metadata records (exit 0)
  • python tools/build_skill_coverage.py --check -> docs/skill-coverage.md matches its sources (exit 0)
  • python -m pytest tests/test_vllm_ascend_distill_skills.py tests/test_knowledge_contract.py tests/test_skill_library.py -> 30 passed (exit 0)

cursor[bot]
cursor Bot previously approved these changes Nov 10, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.json and src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-aclgraph-capture-replay.json.
  • Model-adaptation adds references/source-notes.md, provenance_readsets, and refusal_point. The packaged JSON and coverage row (refusal_point, 1 readset, 3 advice) match. The FP8 scale multiply is labeled an implementation interpretation in both SKILL.md and source-notes. Historical prefix bc8115ed is recorded as bc8115ed7fdc7e500320b5de70742ef049b4c1a2.
  • The host-offload link ../../host/host-memory-management/SKILL.md matches the existing packaged skill path.
  • tests/test_vllm_ascend_distill_skills.py locks 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, and python tools/build_skill_coverage.py --check were not run. CI was not inspected.
  • ACLGraph references/source-notes.md is unchanged (6ffbfb68cc7e04923b4fbca2a73a7555b7c3b45a6f40819bb5f3213ef05cf88d). The new ENPU update/replay order and weak_ref_output sentences were not re-read against F4–F5.
  • The full SHA for bc8115ed was not resolved against vllm-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" under ascend-graph-mode.
  • The FULL / FULL_DECODE_ONLY proposal 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/.

Copy link
Copy Markdown
Collaborator

Re-review (independent; COMMENT only, not an approval)

Reviewed range: previous head 1fb3a4b5e9bb7395ef0ce70dd4407eea05563a3f → current head b205b12ce7ca286c0be11f8bc21639c56c4e09cf. The branch was rebased onto 3f560642a44a1496aa4984ab936517693be2c788, with the content commit amended to b887480e6c393a2e0a1efb0ee5d2b7ddf57bb10a plus the regeneration commit b205b12c. So I re-read the full content diff, not just the last commit.

Earlier findings: status

  1. Generated records: resolved. Both records now carry the new tags, readset, refusal_point and hashes. The pin b887480e → b205b12c touches no skill_library path, so the declared source revision matches the head's library bytes. The other 78 records change only the pin, which is the expected churn under the current format.
  2. Bucketing claims: resolved. Step 3 now sticks to F2/F6 (BatchDescriptor keying, missing entry → capture after validate_cudagraph_capturing_enabled(), NONE/mismatch bypass, capture-size bounds) and places request bucketing upstream of the wrapper. The new metadata.json recommendations match.
  3. ENPU ordering: resolved. synchronize() is gone. Ordering is scoped to the update-aware FULL path (use_updatable_graph(attn_backend)), and the note that the runner caller was not inspected is kept, consistent with F5.
  4. Three-tier text: resolved. It's now labelled an InferRSI routing convention, the FP8 formula is marked as an interpretation with checkpoint-specific scale granularity, and the full bc8115ed… resolution is stated.
  5. Frontmatter: resolved. The description diff is gone.
  6. CI: still open, and this is the blocker. For b205b12c, the main CI — ready / review lane run is in action_required (it needs maintainer approval to run). So full-check, installed-package and lint-and-smoke have not run on this head. CI — knowledge declaration policy has one failed and one later successful run on the same head; the latest is green. I can't approve while the full test suite hasn't run on the head, especially since this changes 80 generated records and adds tests/test_vllm_ascend_distill_skills.py.

Minor (non-blocking)

Next

Once a maintainer approves the workflow run and full-check / installed-package pass on this exact head, I'll re-review and can Approve if nothing else changes.

@Dong1017 Dong1017 added area:knowledge Knowledge line: packs, skill library, knowledge admission kind:task Bounded delivery task with a delegated slice labels Nov 10, 2026

Copy link
Copy Markdown
Collaborator

Incremental review (COMMENT only, not an approval)

Reviewed range: b205b12ce7ca286c0be11f8bc21639c56c4e09cf..d15379faa0ddfca90e5256c860e0f1bc14b93bb8 (commits 620751676f300a0956388b415a295a8dfe6c74da and d15379faa0ddfca90e5256c860e0f1bc14b93bb8). The only change is to src/inferrsi/data/skill_library/hardware/ascend/ascend-model-adaptation/SKILL.md lines 3–4.

Blocking

  1. These two commits bring back the frontmatter regression that b205b12c had already fixed. I parsed the frontmatter with PyYAML safe_load:

    • base 3f560642: description ends with '... or a similar framework.\n'
    • b205b12c: '... or a similar framework.\n', identical to base and correct
    • d15379fa: '... or a similar framework. '. In a single-quoted scalar, the line break before the closing ' folds into a trailing space, so the value now differs from base.

    Please restore the two lines exactly as they are at b205b12c (framework. followed by a blank line and then '), i.e. revert both commits. On b205b12c the description was already byte-identical to base, so nothing here needed fixing.

  2. The generated records are stale again. SKILL.md now hashes to 227824cd9b35a7771a9af7b44aba3d26d0e1ebce8d65af8fe14aa6e649a6108a. But src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-model-adaptation.json still records source_files[...SKILL.md] = 24ca19fc… and pins source revision b887480e. So python tools/build_skill_index.py --source-revision b887480e6c393a2e0a1efb0ee5d2b7ddf57bb10a --check would fail on this head. The clean fix is the revert in item 1. That makes the library bytes match b887480e again, so no re-pin is needed.

  3. CI: unchanged from my previous comment. CI — ready / review lane is still action_required, so full-check and installed-package haven't run on this head. CI — knowledge declaration policy currently shows only a failure run for d15379fa. Please check the declaration block and rerun it on this head.

The other points from my previous comment (6761392302) still stand.

Copy link
Copy Markdown
Collaborator

Incremental review (COMMENT only, not an approval)

Reviewed range: d15379faa0ddfca90e5256c860e0f1bc14b93bb8..ee04168f9499f3686ebf06064f95851e03ebc988 (commit ee04168f). It regenerates the knowledge records and re-pins them to d15379fa.

Resolved

  • Generated records match the library again. ascend-model-adaptation.json now records SKILL.md = 227824cd… and pins d15379fa, which matches the head's SKILL.md bytes. Item 2 of my previous comment (6761591412) is resolved.

Still blocking

  1. The frontmatter regression is still there. src/inferrsi/data/skill_library/hardware/ascend/ascend-model-adaptation/SKILL.md lines 3–4 are unchanged from d15379fa, so description still parses (PyYAML safe_load) to '... or a similar framework. ', with a trailing space. Base 3f560642 and b205b12c both give '... or a similar framework.\n'. The regenerated record now carries that value too ("... or a similar framework. "), so the regression has propagated into the generated knowledge. Please restore the two lines exactly as at b205b12c, then regenerate the index again.
  2. docs/skill-coverage.md is probably stale. tools/build_skill_coverage.py input_digest() hashes the bytes of every SKILL.md under the library (lines 190–235). ascend-model-adaptation/SKILL.md changed after b205b12c, but docs/skill-coverage.md still has the b205b12c digest 3191bbad…, so python tools/build_skill_coverage.py --check should fail on this head. Please regenerate it after fixing item 1. If you think the digest doesn't need to change, please paste the --check output.
  3. CI: CI — ready / review lane is still action_required (it needs maintainer approval to run), so full-check and installed-package haven't run on this head. CI — knowledge declaration policy for this head has only a failure run so far.

The minor points from comment 6761392302 still stand.

Copy link
Copy Markdown
Collaborator

Incremental review (COMMENT only, not an approval)

Reviewed range: ee04168f9499f3686ebf06064f95851e03ebc988..de7f4c804dbb1e60849f3591a1715f3d7fc3e927 (commit de7f4c80).

Resolved

  • Frontmatter: ascend-model-adaptation/SKILL.md is back to the b205b12c bytes (sha256 24ca19fc…), and description again parses to '... or a similar framework.\n', matching base.
  • docs/skill-coverage.md: with the bytes restored, its digest 3191bbad… matches again.

Still blocking

  1. The generated records are stale again, in the opposite direction. At this head, src/inferrsi/data/repo_packs/inferrsi/knowledge/ascend/skills/ascend-model-adaptation.json still records source_files[...SKILL.md] = 227824cd…, pins d15379fa, and carries the description with the trailing space ("... or a similar framework. "). The 80 other records also pin d15379fa, whose library bytes differ from this head. So build_skill_index.py --check should fail. The simplest fix: the library bytes now match b887480e, so restore the generated records and knowledge/README.md to their b205b12c state (i.e. revert ee04168f). Alternatively, regenerate against a revision whose bytes match this head. Either way, please paste the --check exit status and output.
  2. CI (unchanged): CI — ready / review lane is still action_required, and CI — knowledge declaration policy on this head shows only failure.

Suggestion: before pushing, run all three checks locally together (build_skill_index.py --check, build_skill_coverage.py --check, pytest tests/test_vllm_ascend_distill_skills.py). The last few pushes have fixed one generated artifact and broken another.

Copy link
Copy Markdown
Author

Fixed the review blockers in the latest commits.

What changed:

  • Regenerated the complete packaged knowledge index (80 skill records plus README) from committed source revision b205b12ce7ca286c0be11f8bc21639c56c4e09cf.
  • Updated the two Ascend knowledge records so their summaries, source revisions, source hashes, metadata, and provenance match the checked-in skill sources.
  • Preserved the correct ascend-model-adaptation frontmatter value without trailing whitespace.
  • Kept the evidence boundaries from the review: no unsupported dynamic-bucketing or synchronization claims, explicit InferRSI routing-convention wording, semantic-correctness caveat, separate-pin boundaries, and checkpoint-specific FP8 scale handling.

Verification:

  • 20 passed for the targeted test suite.
  • Skill index check passed: 80 metadata records.
  • Ruff check passed.
  • PR head: 798d805; the PR is open and mergeable.

Copy link
Copy Markdown
Author

Updated the knowledge-model-policy declaration to the current PR head 798d805d58a44810b806b5f58cb0c4455d91624f, which was the cause of the stale-declaration failure. The GitHub App cannot rerun the failed workflow because Actions write permission/maintainer approval is required; please approve the pending fork workflow and rerun declarations.

Copy link
Copy Markdown
Collaborator

Incremental review (COMMENT only, not an approval)

Reviewed range: de7f4c804dbb1e60849f3591a1715f3d7fc3e927..798d805d58a44810b806b5f58cb0c4455d91624f (commit 798d805d, 81 generated files). I also compared b205b12ce7ca286c0be11f8bc21639c56c4e09cf..798d805d58a44810b806b5f58cb0c4455d91624f as a whole.

Resolved

  • Generated records. Net of the last five commits, nothing under src/inferrsi/data/skill_library/ differs from b205b12c, so the library bytes at this head are the b205b12c bytes. The 80 records and knowledge/README.md now pin b205b12c, and against b205b12c the only change in them is that pin (b887480e → b205b12c). ascend-model-adaptation.json is back to SKILL.md = 24ca19fc… with description ending in framework.\n, matching base. So the stale-record blocker from my previous comment (6761832474) is resolved, and docs/skill-coverage.md stays consistent since no SKILL.md bytes changed.
  • Declarations. CI — knowledge declaration policy now has a later successful declarations run on this head (07:53 UTC), after the earlier failure.

Still blocking

  1. Full CI has not run on this head. The only check runs on 798d805d are declarations, plan, execute (skipped) and publish. full-check, installed-package and lint-and-smoke have not run, because the CI — ready / review lane workflow for this fork still needs maintainer approval. Since this PR changes 80 generated records and adds tests/test_vllm_ascend_distill_skills.py, I'll Approve only after those pass on this exact head. The author-reported local results (20 passed, index check OK, Ruff OK) are noted but are not a substitute.

Minor (non-blocking, still open from 6761392302)

  • ascend-model-adaptation/references/source-notes.md, "Operational interpretation": drop "generic scheduler bucketing interactions", since the skill no longer covers bucketing.
  • Same file, F3 uses num_kv_heads while SKILL.md uses num_key_value_heads; please pick one name.

If you address the minor points, please regenerate the index in the same push and run build_skill_index.py --check, build_skill_coverage.py --check and the targeted pytest together before pushing.

Copy link
Copy Markdown
Author

Addressed the remaining review follow-ups.

  • Removed the obsolete “generic scheduler bucketing interactions” wording from ascend-model-adaptation/references/source-notes.md.
  • Standardized the F3 terminology to num_key_value_heads.
  • Regenerated all 80 packaged knowledge records and the knowledge README from source commit 79a6014.
  • Updated the declaration to the final PR head 755e548.

Verification:

  • docs/skill-coverage.md matches its sources
  • Skill index is current: 80 metadata records
  • 30 tests passed
  • Ruff passed

The remaining merge blocker is maintainer approval for the fork workflow/full CI.

Copy link
Copy Markdown
Collaborator

Incremental review (COMMENT only, not an approval)

Reviewed range: 798d805d58a44810b806b5f58cb0c4455d91624f..755e548be8d5b27dc40d7c41735431e55be754fe (79a60141 terminology fix + 755e548b index regeneration, 82 files).

Resolved

  • Both minor points from 6762120215. ascend-model-adaptation/references/source-notes.md no longer mentions "generic scheduler bucketing interactions", and F3 now uses num_key_value_heads.
  • Regenerated records are consistent. 755e548b touches only generated files (nothing under src/inferrsi/data/skill_library/), so the library bytes at this head are the 79a60141 bytes. All 80 records and knowledge/README.md pin 79a60141, and the only content change besides the pin is the source-notes.md hash in ascend-model-adaptation.json. I checked that this hash, 2e9d1b85…, matches the file at both 79a60141 and the head. skill-index.json (7904275c…) is unchanged at both, and no SKILL.md changed, so docs/skill-coverage.md stays valid.
  • Declarations. declarations is green on this head.

Still blocking (unchanged)

  1. Full CI has not run on this head. The only check runs on 755e548b are declarations, plan, execute (skipped) and publish. full-check, installed-package and lint-and-smoke are still waiting on maintainer approval of the fork workflow. As before, I'll Approve once they pass on this exact head. The reported local results (30 passed, index and coverage checks OK, Ruff OK) are noted.

Nit (non-blocking)

  • ascend-model-adaptation/SKILL.md is still mixed: L58 says num_key_value_heads, but L64 says tp_size > num_kv_heads. If you fix it, please regenerate the index in the same push.

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

Labels

area:knowledge Knowledge line: packs, skill library, knowledge admission kind:task Bounded delivery task with a delegated slice

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants