Skip to content

test: cover Compressor overlap and non-overlap windowing (PRPUNDIT-25) - #1098

Merged
Xiaoming-AMD merged 2 commits into
mainfrom
testgap/compressor-forward
Sep 6, 2026
Merged

test: cover Compressor overlap and non-overlap windowing (PRPUNDIT-25)#1098
Xiaoming-AMD merged 2 commits into
mainfrom
testgap/compressor-forward

Conversation

@jiagaoxiang

Copy link
Copy Markdown
Collaborator

Same-repo refile of #1085 so Primus-CI-TAS runs against AMD-AGI/Primus (fork PRs do not receive Docker Hub credentials, so build-docker / torch unit tests never ran).

This PR was created by dougljia via Test Gap Resolver.

Summary

  • Adds a dedicated CPU behavioral test for Compressor.forward in tests/unit_tests/backends/megatron/test_compressor_forward.py, not the FP32-policy suite (test_v4_keep_in_fp32.py) and not the fused-pool kernel suite (test_compressor_pool.py).
  • Independent manual oracle for CSA overlap (ratio=4) and HCA non-overlap (ratio=128): window i is [half_a[i], half_b[i-1]] with zeros on window 0, or a contiguous ratio slice, then eager softmax-pool + RMSNorm.
  • Also covers fused vs unfused projections (PRIMUS_COMPRESS_FUSE_PROJ) and the sequence-not-divisible-by-ratio guard, per linxwang's PRPUNDIT-25 note.

Closes test gap PRPUNDIT-25.

Test plan

  • PYTHONPATH=. pytest tests/unit_tests/backends/megatron/test_compressor_forward.py (8 passed, CPU)
  • black --check on the new file

Dedicated CPU behavioral oracle for CSA (ratio=4) and HCA (ratio=128),
kept out of the FP32-policy suite so a stitching bug cannot hide behind
shape/dtype checks.
Linxwang's PRPUNDIT-25 note also asked for the fuse-proj split and the
non-divisible sequence guard alongside the windowing oracle.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The PR adds a well-scoped, CPU-only test suite that aligns with the implementation semantics and improves coverage without changing production code paths.

Pull request overview

Adds a dedicated, CPU-based behavioral oracle test suite for primus.backends.megatron.core.transformer.compressor.Compressor.forward, specifically validating correct window construction for CSA overlap (ratio=4) and HCA non-overlap (ratio=128), plus fused vs. unfused projection equivalence and the sequence-length divisibility guard.

Changes:

  • Introduces a manual (helper-independent) reference implementation for windowing + softmax-pool + RMSNorm to validate Compressor.forward end-to-end on CPU.
  • Covers both overlap and non-overlap behaviors, including a targeted check that overlap window 0 is causally padded (no predecessor influence).
  • Adds a parity test ensuring PRIMUS_COMPRESS_FUSE_PROJ toggling does not change outputs (given equivalent weights), and asserts the expected “sequence not divisible by ratio” failure.
File summaries
File Description
tests/unit_tests/backends/megatron/test_compressor_forward.py New CPU behavioral tests providing an independent oracle for Compressor.forward windowing (overlap/non-overlap), fused/unfused projection parity, and input-length guard behavior.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Checked CI on head 971757f (poll flagged build-docker (3.12) as failed).

That failure is stale: it's from workflow run #4994, which ran against the fork context (jiagaoxiang/Primus) before this PR was refiled into AMD-AGI/Primus directly — exactly the missing-Docker-Hub-credentials situation described in the PR body. The current, authoritative run for this same commit against AMD-AGI/Primus is #5026: code-lint and dependency-review have already passed, and build-docker (3.12) / run-unittest-jax are queued (not failed) there.

No code change needed on my end. I'll keep watching for the new run to finish and for review activity.

@Xiaoming-AMD
Xiaoming-AMD merged commit bdede9f into main Sep 6, 2026
15 of 16 checks passed
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.

3 participants