Skip to content

test(pt): reuse PT2 artifacts for metadata checks - #5812

Draft
njzjz-bot wants to merge 2 commits into
deepmodeling:masterfrom
njzjz-bot:test/reuse-pt2-aot-fixtures
Draft

test(pt): reuse PT2 artifacts for metadata checks#5812
njzjz-bot wants to merge 2 commits into
deepmodeling:masterfrom
njzjz-bot:test/reuse-pt2-aot-fixtures

Conversation

@njzjz-bot

@njzjz-bot njzjz-bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • build the base energy PT2 package once in a module-scoped fixture
  • share it between metadata/ZIP/accessor tests and numerical inference tests
  • derive metadata-only and model-definition variants by editing PT2 ZIP extras instead of recompiling AOTInductor
  • preserve a separate PTE reference for cross-format checks

Validation

  • 22 PT2 metadata and inference tests passed in 90.15 seconds
  • the only expensive phase was one 84.02-second fixture setup
  • ruff check .
  • ruff format .

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

Summary by CodeRabbit

  • Tests
    • Improved DeepEval PT2/PTE test setup by sharing compiled artifacts across test cases.
    • Added coverage for metadata-only archives and training configuration handling.
    • Updated export, serialization, and archive validation tests to use reusable fixtures.
    • Added safeguards to ensure artifact contents remain unchanged except for intended metadata updates.

Build the base AOTInductor package once in a module-scoped fixture, derive metadata variants by editing ZIP extras, and share the artifact between metadata and inference test classes.\n\nCoding-Agent: Codex\nCodex-Version: codex-cli 0.144.4\nModel: gpt-5.6-sol\nReasoning-Effort: xhigh
@dosubot dosubot Bot added the build label Jul 16, 2026
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 88b79fb9-3aee-44ca-a0ea-8788e25d867f

📥 Commits

Reviewing files that changed from the base of the PR and between 1c5ff68 and 0cf6550.

📒 Files selected for processing (1)
  • source/tests/pt_expt/infer/test_deep_eval.py

📝 Walkthrough

Walkthrough

The deep evaluation tests replace per-class artifact setup with shared pytest fixtures. Helpers create PT2/PTE variants, metadata assertions use shared archives, and deep evaluation tests consume fixture-provided artifact paths.

Changes

Deep evaluation artifact tests

Layer / File(s) Summary
Shared artifact construction
source/tests/pt_expt/infer/test_deep_eval.py
Adds helpers and pytest fixtures that compile shared PT2/PTE artifacts and create metadata variants by editing selected archive entries.
Metadata archive validation
source/tests/pt_expt/infer/test_deep_eval.py
Updates metadata tests to inspect shared archives and compare configured model metadata with the fixture-provided training configuration.
Deep evaluation fixture integration
source/tests/pt_expt/infer/test_deep_eval.py
Injects shared artifact context into TestDeepEvalEnerPt2 and uses shared PT2/PTE paths for model loading and serialization tests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested labels: enhancement, Python

Suggested reviewers: wanghan-iapcm

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the change to reuse PT2 artifacts, though it highlights only the metadata side of a broader test refactor.
Docstring Coverage ✅ Passed Docstring coverage is 94.12% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.96%. Comparing base (4f827cc) to head (c9df92e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5812      +/-   ##
==========================================
- Coverage   79.21%   78.96%   -0.25%     
==========================================
  Files        1069     1069              
  Lines      124070   124070              
  Branches     4522     4527       +5     
==========================================
- Hits        98278    97969     -309     
- Misses      24171    24484     +313     
+ Partials     1621     1617       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@njzjz

njzjz commented Jul 16, 2026

Copy link
Copy Markdown
Member

The CI timing shows that this optimization is effective for the targeted tests, but the workflow-level benefit is limited.

Comparing the Python 3.10 duration artifacts from the base SHA 1c5ff68 (run 29456447617) with the PR head 0cf6550 (run 29468453118):

  • The 22 TestDeepEvalEnerPt2* tests decreased from 2306.7 s to 1942.0 s, saving 364.7 s (15.8%, about 6.1 runner-minutes).
  • These tests occupied all 12 CI splits on the base run and 10 splits on the PR run. The module-scoped fixture is process-local, so AOT compilation still occurs once in every split containing one of these tests, rather than once for the entire CI workflow.
  • Across all 12 Python 3.10 pytest jobs, aggregate test time changed from 36,727 s to 38,092 s, while the slowest shard changed from 3,426 s to 3,594 s. Whole-suite noise and split rebalancing therefore dominate the workflow wall-clock result; this run does not demonstrate a critical-path speedup.

Conclusion: the PR is a valid targeted optimization, with a measured ~16% reduction for the affected tests in this CI run, but it should not be interpreted as compiling the fixture only once across CI or as an established whole-workflow speedup. It would be useful to recheck after the updated duration cache has rebalanced the splits.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

@njzjz

njzjz commented Jul 18, 2026

Copy link
Copy Markdown
Member

Possible reviewers based on changed lines, exact file history, and exact-file review history:

  • @wanghan-iapcm — 8 commits on changed files; 40 reviews on exact changed files (source/tests/pt_expt/infer/test_deep_eval.py).
  • @iProzd — 6 reviews on exact changed files (source/tests/pt_expt/infer/test_deep_eval.py).

No review request was made automatically.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

@njzjz
njzjz marked this pull request as draft July 18, 2026 07:03
@njzjz

njzjz commented Jul 18, 2026

Copy link
Copy Markdown
Member

Moving this PR back to draft because the current CI result does not demonstrate an overall CI speedup.

The targeted 22 PT2 tests improved from 2306.7 s to 1942.0 s of cumulative test time (15.8%), but the CI command uses 12 pytest splits. The module-scoped fixture is local to each pytest process, so the PT2 artifact was still compiled independently in 10 shards rather than once. In the available paired run, the cumulative duration of the 12 main pytest steps increased from 36,727 s to 38,092 s, and the slowest shard increased from 3,426 s to 3,594 s. Runner noise means this does not prove a regression, but it also does not establish the intended CI acceleration.

Before marking this ready, the tests should either share a prebuilt artifact across shards, be kept in a dedicated/single shard, or otherwise be validated with repeated CI runs showing a shorter critical path.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

@njzjz
njzjz requested review from iProzd and wanghan-iapcm and removed request for iProzd and wanghan-iapcm July 18, 2026 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants