Skip to content

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

Closed
jiagaoxiang wants to merge 2 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/compressor-forward
Closed

test: cover Compressor overlap and non-overlap windowing (PRPUNDIT-25)#1085
jiagaoxiang wants to merge 2 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/compressor-forward

Conversation

@jiagaoxiang

@jiagaoxiang jiagaoxiang commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

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.
Copilot AI lite review requested due to automatic review settings September 3, 2026 16:19

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 is test-only and the new tests accurately mirror the production windowing/pooling/RMSNorm logic while adding concrete coverage for overlap vs non-overlap behavior and a key input-shape guard.

Pull request overview

Adds a focused CPU unit test suite that validates primus.backends.megatron.core.transformer.compressor.Compressor.forward behavior for both CSA overlap (ratio=4) and HCA non-overlap (ratio=128) by comparing against an independent, hand-derived windowing + softmax-pool + RMSNorm oracle.

Changes:

  • Introduces tests/unit_tests/backends/megatron/test_compressor_forward.py with a manual reference implementation for overlap and non-overlap window construction.
  • Adds behavioral assertions that (a) forward matches the independent oracle, (b) overlap window 0 uses causal zero padding (no predecessor leakage), and (c) contiguous-mode forward asserts when S % ratio != 0.
File summaries
File Description
tests/unit_tests/backends/megatron/test_compressor_forward.py New CPU behavioral tests that independently validate overlap/non-overlap windowing semantics and forward output against a manual oracle.
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.

Linxwang's PRPUNDIT-25 note also asked for the fuse-proj split and the
non-divisible sequence guard alongside the windowing oracle.
@jiagaoxiang jiagaoxiang changed the title test: cover Compressor overlap and non-overlap windowing test: cover Compressor overlap and non-overlap windowing (PRPUNDIT-25) Sep 3, 2026
Copilot AI review requested due to automatic review settings September 3, 2026 16:32

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 new tests are consistent with the current Compressor implementation and meaningfully close the stated behavioral test gap without introducing risky production changes.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Checked the build-docker (3.12) failure on head 971757f (run 33779247129).

The image build itself completes fine (all layers cached, image tagged docker.io/tasimage/primus:pr-1085). The job then fails on:

flag needs an argument: 'p' in -p
Usage:  docker login [OPTIONS] [SERVER]
##[error]Process completed with exit code 125.

This comes from the docker login -u rocmshared -p ${{ secrets.ROCM_DOCKER_HUB_TOKEN }} step in .github/workflows/ci.yaml. Since this PR's head branch runs from a fork (jiagaoxiang/Primus), ROCM_DOCKER_HUB_TOKEN isn't available to the pull_request workflow run (GitHub withholds repo secrets from fork PRs), so -p gets an empty value and docker login errors out.

This is a repo-wide CI/secrets-config issue, not something introduced by this test-only change (the diff only adds tests/unit_tests/backends/megatron/test_compressor_forward.py), and it isn't a transient flake — rerunning would fail the same way for any fork PR. I'm leaving it as-is rather than touching the workflow (out of scope for a test PR) or repeatedly rerunning a job that can't succeed under these permissions. A maintainer will need to either scope that login step to non-fork runs or make the Docker Hub push conditional.

The two Copilot reviews are both "approval recommended" with zero comments, so there's nothing to address there. Still waiting on a human approval and a passing/waived build-docker check before this is mergeable.

@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Checked in on this PR after the poll flagged failed CI + reviews.

  • CI (build-docker (3.12), run 33779247129): the job fails at the docker login -u tasimage -p step with flag needs an argument: 'p' in -p — the password argument is empty, so the Docker Hub credential/secret isn't reaching this step. This happens after the image itself builds successfully (all layers CACHED/built fine), so it's a CI credential/secret plumbing issue on the runner side, not something caused by this PR's change (a single new test file, tests/unit_tests/backends/megatron/test_compressor_forward.py). This looks like repo-wide CI infrastructure breakage rather than a PR-caused failure or a simple flake, so I'm not rerunning it or touching CI config — a maintainer with access to the Docker Hub secret/runner config should look at it.
  • Reviews: both reviews so far are from copilot-pull-request-reviewer[bot] (🟢 approval recommended, 0 line comments on either pass) — no findings to address, and bot approval doesn't count toward mergeability. Still waiting on a human review.

No code changes needed on my end right now. Will keep watching for a human review and for the build-docker check to be fixed/rerun by a maintainer.

@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

Superseded by same-repo PR #1098 (head AMD-AGI:testgap/compressor-forward) so Primus-CI-TAS receives Docker Hub credentials and can run torch unit tests. Closing this fork PR.

@jiagaoxiang jiagaoxiang closed this Sep 4, 2026
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.

2 participants