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.

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

feat!: Move passes from algorithms into a separate crate #1100

Merged
merged 30 commits into from
May 23, 2024
Merged

Conversation

cqc-alec
Copy link
Collaborator

@cqc-alec cqc-alec commented May 23, 2024

Closes #1000 .

Also:

  • Moved some tests that were making use of these passes (or their test code) from the hugr crate into the hugr-passes crate.
  • Moved some functions from test code into the utils module and made it public.
  • Made Hugr::new() public. I thought I could avoid this by using the CFGBuilder but doing so caused some extension-inference tests to fail, for reasons I couldn't fathom.

Do we want to update the release-plz configuration? Not sure exactly what to do here.

BREAKING CHANGE: The algorithms module is removed from hugr. It's functions are now in a separate hugr-passes crate.

@cqc-alec cqc-alec changed the title feat!: Move passes from algorithms into a separat crate feat!: Move passes from algorithms into a separate crate May 23, 2024
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 94.48276% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 86.82%. Comparing base (4b8d23d) to head (8840073).

Files Patch % Lines
hugr-passes/src/const_fold.rs 85.29% 14 Missing and 6 partials ⚠️
hugr-passes/src/const_fold/test.rs 98.44% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1100      +/-   ##
==========================================
+ Coverage   86.81%   86.82%   +0.01%     
==========================================
  Files          90       90              
  Lines       18501    18502       +1     
  Branches    18108    18109       +1     
==========================================
+ Hits        16062    16065       +3     
  Misses       1600     1600              
+ Partials      839      837       -2     
Flag Coverage Δ
rust 86.90% <94.48%> (+0.01%) ⬆️

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.

@cqc-alec cqc-alec marked this pull request as ready for review May 23, 2024 09:57
@cqc-alec cqc-alec requested a review from a team as a code owner May 23, 2024 09:57
@cqc-alec cqc-alec requested review from zrho and aborgna-q and removed request for zrho May 23, 2024 09:57
Copy link
Collaborator

@aborgna-q aborgna-q left a comment

Choose a reason for hiding this comment

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

I have multiple comments about CI and metadata config, but it looks good.

hugr-passes/Cargo.toml Outdated Show resolved Hide resolved
hugr-passes/src/const_fold/int_ops_const_fold_test.rs Outdated Show resolved Hide resolved
hugr-passes/src/const_fold.rs Outdated Show resolved Hide resolved
hugr-passes/src/lib.rs Outdated Show resolved Hide resolved
@@ -140,7 +140,6 @@
// https://github.com/proptest-rs/proptest/issues/447
#![cfg_attr(test, allow(non_local_definitions))]

pub mod algorithm;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once we do #294 we should re-export algorithms here.

Normally we sub-crates are used for specific components of the library and the main library re-exports all non-internal things, but that will have to wait for now.

hugr-passes/Cargo.toml Show resolved Hide resolved
hugr-passes/Cargo.toml Show resolved Hide resolved
Comment on lines 3 to 4
version = "0.1.0"
edition = "2021"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
version = "0.1.0"
edition = "2021"
version = "0.4.0"
edition = { workspace = true }
rust-version = { workspace = true }
license = { workspace = true }
readme = "README.md"
documentation = "https://docs.rs/hugr-passes/"
homepage = { workspace = true }
repository = { workspace = true }
description = "Compiler passes for Quantinuum's HUGR"
keywords = ["Quantum", "Quantinuum"]
categories = ["compilers"]

Let's use the same major/minor version for all packages, to avoid confusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that case I can add version under [workspace.package] in the top-level Cargo.toml and use {workspace = true } in both packages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we want to have to make a new release of hugr whenever we do a new release of hugr-passes? This shouldn't be necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about having a matching minor number (0.x), even if the patch version varies.
But it may get desynchronised too, so I guess it's fine to leave it as 0.1.0.

The releases are done synchronously, but only for packages with changes.
See this PR for an example on how this looks.

hugr-passes/Cargo.toml Outdated Show resolved Hide resolved
github-merge-queue bot pushed a commit that referenced this pull request May 23, 2024
Automated change generated by
[`cargo-autoinherit`](https://github.com/mainmatter/cargo-autoinherit).

This should simplify adding common dependencies on the new subcrates
(#1100).
@aborgna-q
Copy link
Collaborator

Ah, and two more tasks:

  • Add the to-be-created hugr-passes/CHANGELOG.md to .github/CODEOWNERS
  • Add the new directory to the rust section in .github/change-filters.yml

@cqc-alec cqc-alec requested a review from aborgna-q May 23, 2024 12:56
hugr-passes/README.md Outdated Show resolved Hide resolved
Co-authored-by: Agustín Borgna <[email protected]>
@cqc-alec
Copy link
Collaborator Author

cqc-alec commented May 23, 2024

Just noticed that the top-level CHANGELOG.md is a symlink to hugr/CHANGELOG.md. Should we delete the symlink?

@cqc-alec
Copy link
Collaborator Author

Just noticed that the top-level CHANGELOG.md is a symlink to hugr/CHANGELOG.md. Should we delete the symlink?

Would then need to update the link in the hugr/README.md.

@aborgna-q
Copy link
Collaborator

Yes! We should also replace the README symlink with a root README explaining the different crates, but that can be done later

hugr-passes/Cargo.toml Outdated Show resolved Hide resolved
@aborgna-q
Copy link
Collaborator

Btw, the token used by release-plz does not have permission for publishing new crates on crates.io, so the first publish must be manual.
(And then we'll have to add the new crate to the crates.io token's list)

@cqc-alec cqc-alec added this pull request to the merge queue May 23, 2024
Merged via the queue into main with commit 3e17193 May 23, 2024
18 checks passed
@cqc-alec cqc-alec deleted the ae/passes-crate branch May 23, 2024 13:21
This was referenced May 23, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 28, 2024
Moves all the code from `hugr` into a `hugr-core` subcrate.
This way, `hugr-passes` (and any other new subcrate) can depend on the
core definitions and get reexported in `hugr`.

This PR does not. Change any definition or visibility in the code, as
the renamed files are noisy enough.
In particular, having `hugr-core` will let us unseal `HugrInternals` and
`HugrMutInternals` so they can be used in `tket2`, but that's work for
another PR.

As a bonus, #1100 is no longer a breaking change since we can re-export
the library without a cyclic dependency.

Closes #294
This was referenced May 29, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 29, 2024
## 🤖 New release
* `hugr`: 0.4.0 -> 0.5.0
* `hugr-cli`: 0.1.0
* `hugr-core`: 0.1.0
* `hugr-passes`: 0.1.0

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

## `hugr`
<blockquote>

## 0.5.0 (2024-05-29)

### Bug Fixes

- Missing re-exports in `hugr::hugr`
([#1127](#1127))
- Set initial version of hugr-core to 0.1.0
([#1129](#1129))

### Features

- [**breaking**] Remove `PartialEq` impl for `ConstF64`
([#1079](#1079))
- [**breaking**] Allow "Row Variables" declared as List<Type>
([#804](#804))
- Hugr binary cli tool ([#1096](#1096))
- [**breaking**] Move passes from `algorithms` into a separate crate
([#1100](#1100))
- [**breaking**] Disallow nonlocal value edges into FuncDefn's
([#1061](#1061))
- [**breaking**] Move cli in to hugr-cli sub-crate
([#1107](#1107))
- Add verbosity, return `Hugr` from `run`.
([#1116](#1116))
- Unseal and make public the traits `HugrInternals` and
`HugrMutInternals` ([#1122](#1122))

### Refactor

- [**breaking**] No Ports in TypeRow
([#1087](#1087))
- Add a `hugr-core` crate
([#1108](#1108))
</blockquote>

## `hugr-core`
<blockquote>

## 0.1.0 (2024-05-29)

### Bug Fixes

- Set initial version of hugr-core to 0.1.0
([#1129](#1129))

### Features

- [**breaking**] Move cli in to hugr-cli sub-crate
([#1107](#1107))
- Make internals of int ops and the "int" CustomType more public.
([#1114](#1114))
- Unseal and make public the traits `HugrInternals` and
`HugrMutInternals` ([#1122](#1122))

### Refactor

- Add a `hugr-core` crate
([#1108](#1108))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

---------

Co-authored-by: Agustin Borgna <[email protected]>
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.

Make separate hugr-passes crate, depending on hugr
2 participants