Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add ValidationLevel tooling and apply to constant_fold_pass #1035

Merged
merged 5 commits into from
Jun 6, 2024
Merged

feat: Add ValidationLevel tooling and apply to constant_fold_pass #1035

merged 5 commits into from
Jun 6, 2024

Conversation

doug-q
Copy link
Collaborator

@doug-q doug-q commented May 14, 2024

No description provided.

@doug-q doug-q requested a review from ss2165 May 14, 2024 09:57
@doug-q
Copy link
Collaborator Author

doug-q commented May 14, 2024

Includes #1030, let's ensure we merge that first.

@doug-q doug-q force-pushed the doug/verify-level branch 2 times, most recently from f471f91 to a950841 Compare May 14, 2024 10:00
@doug-q doug-q changed the base branch from main to feat/const-fold-validate May 14, 2024 10:01
@doug-q doug-q force-pushed the doug/verify-level branch from a950841 to a475120 Compare May 14, 2024 10:02
ss2165
ss2165 reviewed May 14, 2024
View reviewed changes
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

This looks nice, just a query about naming consistency

hugr/src/algorithm/verify.rs Outdated
/// post verification.
///
/// The default level is `None` because verification can be expensive.
pub enum VerifyLevel {
Copy link
Member

Choose a reason for hiding this comment

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

is there a clear distinction between "validation" and "verification"? Should this be called "ValidateLevel" for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a clear one, but on reflection I think you verify a pass by validating a hugr. Happy to change this if you feel strongly, otherwise will sleep on it and edit.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, though I worry people would expect some kind of formal verification on passes. I think for something similar in tket1 we used the term "audit" though I don't like that much either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have renamed to "Validation", because it does literally "validate".

Base automatically changed from feat/const-fold-validate to main May 14, 2024 10:27
@doug-q doug-q force-pushed the doug/verify-level branch from a475120 to 6628b0f Compare May 14, 2024 10:42
@codecov Codecov
Copy link

codecov bot commented May 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.75000% with 20 lines in your changes missing coverage. Please review.

Project coverage is 87.03%. Comparing base (e993580) to head (3993a2d).

Files Patch % Lines
hugr-passes/src/validation.rs 69.23% 6 Missing and 6 partials ⚠️
hugr-passes/src/const_fold.rs 68.00% 7 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1035      +/-   ##
==========================================
- Coverage   87.06%   87.03%   -0.04%     
==========================================
  Files          95       96       +1     
  Lines       19162    19207      +45     
  Branches    18353    18398      +45     
==========================================
+ Hits        16684    16717      +33     
- Misses       1629     1637       +8     
- Partials      849      853       +4
Flag Coverage Δ
rust 86.93% <68.75%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@doug-q
Copy link
Collaborator Author

doug-q commented May 14, 2024

I changed the type of the pass closure to pass: impl FnOnce(&mut H, &VerfiyLevel) -> Result<T, E> because I think it's a better template for a pass transformer. Obviously for the simpleton VerifyLevel this is not needed, but for a general pass transformer:

  • you want to have the ability to run sub-passes from inside the closure
  • you want to be able to Display the transformer
  • you don't want to be forced to clone the transformer

@doug-q doug-q linked an issue Jun 5, 2024 that may be closed by this pull request
Add a Verify wrapper for passes #1158
Closed
@doug-q doug-q force-pushed the doug/verify-level branch from 6628b0f to 3993a2d Compare June 6, 2024 09:02
@doug-q doug-q requested a review from a team as a code owner June 6, 2024 09:02
@doug-q doug-q requested a review from ss2165 June 6, 2024 09:02
@doug-q doug-q changed the title feat: Add VerifyLevel tooling and apply to constant_fold_pass feat: Add ValidationLevel tooling and apply to constant_fold_pass Jun 6, 2024
@doug-q
Copy link
Collaborator Author

doug-q commented Jun 6, 2024

@ss2165 please have another look.

ss2165
ss2165 approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

thanks!

@doug-q doug-q added this pull request to the merge queue Jun 6, 2024
Merged via the queue into main with commit 6eb6d56 Jun 6, 2024
20 of 21 checks passed
@doug-q doug-q deleted the doug/verify-level branch June 6, 2024 10:32
This was referenced Jun 6, 2024
Closed
Closed
Closed
@hugrbot hugrbot mentioned this pull request Jun 7, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 7, 2024
@hugrbot
chore: release (#1180)
71509d9 
## 🤖 New release
* `hugr`: 0.5.0 -> 0.5.1
* `hugr-core`: 0.1.0 -> 0.2.0
* `hugr-passes`: 0.1.0 -> 0.2.0
* `hugr-cli`: 0.1.0 -> 0.1.1

<details><summary><i><b>Changelog</b></i></summary><p>

## `hugr`
<blockquote>

## 0.5.1 (2024-06-07)

### Refactor

- Move binary to hugr-cli
([#1134](#1134))
</blockquote>

## `hugr-core`
<blockquote>

## 0.2.0 (2024-06-07)

### Bug Fixes

- [**breaking**] Validate that control-flow outputs have exactly one
successor ([#1144](#1144))
- Do not require matching extension_reqs when creating a replacement
([#1177](#1177))

### Features

- Add `ConstExternalSymbol` to prelude
([#1123](#1123))
- `HugrView::extract_hugr` to extract regions into owned hugrs.
([#1173](#1173))

### Testing

- Serialisation round trip testing for `OpDef`
([#999](#999))
</blockquote>

## `hugr-passes`
<blockquote>

## 0.2.0 (2024-06-07)

### Features

- Add `ValidationLevel` tooling and apply to `constant_fold_pass`
([#1035](#1035))
</blockquote>

## `hugr-cli`
<blockquote>

## 0.1.1 (2024-06-07)

### Features

- Reexport `clap::Parser` and `clap_verbosity_flag::Level` from hugr_cli
([#1146](#1146))

### Refactor

- Move binary to hugr-cli
([#1134](#1134))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).
@hugrbot hugrbot mentioned this pull request Jun 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ss2165 ss2165 ss2165 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a Verify wrapper for passes
2 participants
@doug-q @ss2165