Skip to content

[pass] Check post conditions on the resulting model #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 16, 2025

Conversation

justinchuby
Copy link
Member

A bug discovered in #77 where we are checking post conditions on the input model and not the resulting model. This PR fixes it.

Fixes #77

@justinchuby justinchuby requested review from titaiwangms and a team as code owners June 14, 2025 14:19
@justinchuby justinchuby requested a review from Copilot June 14, 2025 14:19
@justinchuby justinchuby enabled auto-merge (squash) June 14, 2025 14:19
Copilot

This comment was marked as outdated.

Copy link

codecov bot commented Jun 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.70%. Comparing base (f173ade) to head (d251d2f).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   73.59%   73.70%   +0.11%     
==========================================
  Files          37       37              
  Lines        4492     4492              
  Branches      902      902              
==========================================
+ Hits         3306     3311       +5     
+ Misses        858      853       -5     
  Partials      328      328              

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

@justinchuby
Copy link
Member Author

@codecov-ai-reviewer test

Copy link
Contributor

codecov-ai bot commented Jun 14, 2025

On it! Codecov is generating unit tests for this PR.

@codecov-ai codecov-ai bot mentioned this pull request Jun 14, 2025
justinchuby pushed a commit that referenced this pull request Jun 14, 2025
This PR adds tests for #78

### Commits:
- Add unit tests for postcondition checking bug fix

Tests verify that ensures() is called with result.model instead of input
model,
covering the fix in PassBase.__call__ where postconditions should check
the
transformed model rather than the original input model.
- Add unit tests for postcondition checking bug fix

Tests verify that ensures() is called with result.model instead of input
model,
covering the fix in PassBase.__call__ where postconditions should check
the
transformed model rather than the original input model.

Co-authored-by: codecov-ai[bot] <156709835+codecov-ai[bot]@users.noreply.github.com>
@justinchuby justinchuby disabled auto-merge June 14, 2025 14:33
Copy link
Contributor

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

This PR adds tests for #78

### Commits:
- Add unit tests for postcondition checking bug fix

Tests verify that ensures() is called with result.model instead of input
model,
covering the fix in PassBase.__call__ where postconditions should check
the
transformed model rather than the original input model.
- Add unit tests for postcondition checking bug fix

Tests verify that ensures() is called with result.model instead of input
model,
covering the fix in PassBase.__call__ where postconditions should check
the
transformed model rather than the original input model.

Co-authored-by: codecov-ai[bot] <156709835+codecov-ai[bot]@users.noreply.github.com>
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where postconditions were incorrectly checked against the input model instead of the resulting model. It updates the internal pass infrastructure and adds tests to ensure that both in-place and non in-place passes properly call ensures() on the correct model, while also verifying the correct exception behavior.

  • Updated _pass_infra.py to call ensures() with result.model.
  • Added tests in _pass_infra_test.py to verify ensures() for non in-place, in-place, and functional passes.
  • Added tests to check proper exception propagation when ensures() fails.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/onnx_ir/passes/_pass_infra_test.py New tests added to verify that ensures() is called with the resulting model and error handling.
src/onnx_ir/passes/_pass_infra.py Updated to call ensures() with result.model to fix the bug reported in issue #77.

@justinchuby justinchuby added this to the 0.1.2 milestone Jun 15, 2025
@justinchuby justinchuby merged commit 4a816f3 into main Jun 16, 2025
24 checks passed
@justinchuby justinchuby deleted the justinchuby-patch-2 branch June 16, 2025 18:15
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.

Checking post-conditions on the original model?
3 participants