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

chore!: Update hugr to 0.8.0 #454

Merged
merged 32 commits into from
Sep 6, 2024
Merged

chore!: Update hugr to 0.8.0 #454

merged 32 commits into from
Sep 6, 2024

Conversation

aborgna-q
Copy link
Collaborator

@aborgna-q aborgna-q commented Sep 4, 2024

Updates guppy to use extension operation definitions instead of opaque ops defined on the fly.

Some relevant changes included:

  • Defined a new guppylang extension with two ops: partial and unsupported
  • Most list and array operations currently emit unsupported, as they require custom hugr building code (the core ops in std.collections don't match the python operations 1 to 1, it'll need some custom compilation code).
  • Changed module.compile to produce a Package containing the hugr along with the required extensions to interpret it. (Currently hardcoded, we should infer which extensions are needed at some point).
  • We hardcode the tket2 extension jsons. The plan is for tket2 to publish a new library exporting those, but for now we need the jsons for validation.
  • Use the prelude operations exported by hugr for most int/float/logic/conversion builtin definitions.

The tket2 dependency is currently pinned to a commit hash, I'll update it once 0.3.0 is released.

BREAKING CHANGE: Bumped the hugr dependency to 0.8.0

@mark-koch mark-koch mentioned this pull request Sep 5, 2024
@aborgna-q aborgna-q changed the base branch from main to ab/hugr-cli-validator September 5, 2024 14:05
@aborgna-q aborgna-q requested a review from mark-koch September 5, 2024 14:47
@aborgna-q aborgna-q marked this pull request as ready for review September 5, 2024 14:47
@aborgna-q aborgna-q requested a review from a team as a code owner September 5, 2024 14:47
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 86.74948% with 64 lines in your changes missing coverage. Please review.

Project coverage is 91.89%. Comparing base (1d0667b) to head (48cf387).

Files with missing lines Patch % Lines
guppylang/prelude/_internal/compiler/arithmetic.py 61.42% 27 Missing ⚠️
guppylang/compiler/hugr_extension.py 74.66% 19 Missing ⚠️
guppylang/prelude/_internal/compiler/list.py 69.69% 10 Missing ⚠️
guppylang/prelude/_internal/compiler/array.py 95.06% 4 Missing ⚠️
guppylang/compiler/expr_compiler.py 95.00% 1 Missing ⚠️
guppylang/prelude/_internal/compiler/prelude.py 96.77% 1 Missing ⚠️
guppylang/prelude/_internal/util.py 96.15% 1 Missing ⚠️
guppylang/tys/param.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #454      +/-   ##
==========================================
+ Coverage   90.40%   91.89%   +1.48%     
==========================================
  Files          49       55       +6     
  Lines        5525     5699     +174     
==========================================
+ Hits         4995     5237     +242     
+ Misses        530      462      -68     

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

def __pow__(self: nat, other: nat) -> nat: ...

@guppy.hugr_op(builtins, int_op("iadd"), ReversingChecker())
def __radd__(self: nat, other: nat) -> nat: ...

@guppy.hugr_op(builtins, int_op("rand"), ReversingChecker())
Copy link
Collaborator Author

@aborgna-q aborgna-q Sep 5, 2024

Choose a reason for hiding this comment

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

An upside of this change is that we can now detect these mislabels at compilation time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

😅

@@ -1,14 +1,6 @@
from tests.util import compile_guppy


import pytest

pytest.skip(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All these pytest.skip were introduced in #455.

@aborgna-q aborgna-q linked an issue Sep 5, 2024 that may be closed by this pull request
Copy link
Collaborator

@mark-koch mark-koch left a comment

Choose a reason for hiding this comment

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

Nice! 🎉

Just a few nits

) -> None:
generic_builder = cast(_DfBase[ops.DfParentOp], builder)
generic_builder = cast(DfBase[ops.DfParentOp], builder)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this cast still needed?

Comment on lines 46 to 53
closure_ty = ht.FunctionType(
captured_types + [v.ty.to_hugr() for v in func.ty.inputs],
[ty.to_hugr() for ty in type_to_row(func.ty.output)],
)
hugr_closure_ty: ht.FunctionType = closure_ty.to_hugr()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to go via FunctionType.to_hugr as before since this also takes care of inout returns

guppylang/prelude/_internal/compiler/arithmetic.py Outdated Show resolved Hide resolved
guppylang/prelude/_internal/compiler/arithmetic.py Outdated Show resolved Hide resolved
Comment on lines +89 to +91
# ------------------------------------------------------
# --------- Custom compilers for non-native ops --------
# ------------------------------------------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think all functions in this section can now be implemented via Guppy source!

Happy to leave that to another PR though, whichever way you prefer

Copy link
Collaborator Author

@aborgna-q aborgna-q Sep 6, 2024

Choose a reason for hiding this comment

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

Yes, let's avoid bloating this PR even more. See issue #464

Comment on lines +33 to +35
# ------------------------------------------------------
# --------- Custom compilers for non-native ops --------
# ------------------------------------------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can also be Guppy source now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

guppylang/prelude/_internal/util.py Outdated Show resolved Hide resolved
guppylang/prelude/_internal/util.py Outdated Show resolved Hide resolved
guppylang/prelude/_internal/util.py Outdated Show resolved Hide resolved
def __pow__(self: nat, other: nat) -> nat: ...

@guppy.hugr_op(builtins, int_op("iadd"), ReversingChecker())
def __radd__(self: nat, other: nat) -> nat: ...

@guppy.hugr_op(builtins, int_op("rand"), ReversingChecker())
Copy link
Collaborator

Choose a reason for hiding this comment

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

😅

github-merge-queue bot pushed a commit that referenced this pull request Sep 6, 2024
Spinoff of #454 

Replaces the validator python library with a small binary that calls out
to `hugr-cli validate`.

`cargo install` lets us download tools like `hugr-cli` and use them for
local development, but the support for locally scoped tools is quite
flaky. By default, tools are installed in a global directory using the
latest version available.
It can be forced to use a local target directory via an env variable /
`cargo` config (which didn't always work while I was testing it), and
version selection can only be done by passing explicit arguments to the
cmd. As there is no central `Cargo.toml` (or similar) config for it,
this relies on every usage point always passing exactly the same
arguments. Updating the tool version / patching in a git ref ends up
being quite error prone.

The solution in this PR is inspired by cargo's own `xtask-` subcrates.
These are internal crates meant to run some external tool, while
centrally defining the dependency versions and artifact paths.

The new `validator` bin crate here checks if `hugr-cli` is installed,
and calls `cargo install` with the appropriate parameters otherwise,
before running the tool. The version to install matches the one
configured in `Cargo.toml`. This ensures we control the validation in
the same way as all the other hugr dependencies (e.g. the `hugr` dep
used in `execute_llvm`).

Running `cargo run` now acts the same as executing a locally-versioned
`hugr-cli`.

drive-by: Created a cargo workspace.

note: I temporarily disabled the tests that required the "collections"
extension definition. This will get fixed once we merge #454, I just
didn't want to wire in a hacky solution that'll get dropped in the next
PR.

Closes #390

---------

Co-authored-by: Craig Roy <[email protected]>
Co-authored-by: Agustin Borgna <[email protected]>
Base automatically changed from ab/hugr-cli-validator to main September 6, 2024 14:37
@aborgna-q aborgna-q added this pull request to the merge queue Sep 6, 2024
Merged via the queue into main with commit b02e0d0 Sep 6, 2024
3 checks passed
@aborgna-q aborgna-q deleted the ab/update-hugr branch September 6, 2024 14:58
github-merge-queue bot pushed a commit that referenced this pull request Sep 11, 2024
🤖 I have created a release *beep* *boop*
---


## [0.10.0](v0.9.0...v0.10.0)
(2024-09-11)


### ⚠ BREAKING CHANGES

* Bumped the `hugr` dependency to `0.8.0`
* `GuppyModule.load` no longer loads the content of modules but instead
just brings the name of the module into scope. Use
`GuppyModule.load_all` to get the old behaviour.
* Removed `guppylang.hugr_builder.hugr.Hugr`, compiling a module returns
a `hugr.Package` instead.

### Features

* Add `__version__` field to guppylang
([#473](#473))
([b996c62](b996c62))
* Add angle type ([#449](#449))
([12e41e0](12e41e0))
* Add array literals
([#446](#446))
([a255c02](a255c02))
* Add equality test for booleans
([#394](#394))
([dd702ce](dd702ce)),
closes [#363](#363)
* Add pi constant ([#451](#451))
([9d35a78](9d35a78))
* Add qualified imports and make them the default
([#443](#443))
([553ec51](553ec51))
* Allow calling of methods
([#440](#440))
([5a59da3](5a59da3))
* Allow imports of function definitions and aliased imports
([#432](#432))
([e23b666](e23b666))
* Array indexing ([#415](#415))
([2199b48](2199b48)),
closes [#421](#421)
[#422](#422)
[#447](#447)
* Inout arguments ([#311](#311))
([060649b](060649b)),
closes [#315](#315)
[#316](#316)
[#349](#349)
[#344](#344)
[#321](#321)
[#331](#331)
[#350](#350)
[#340](#340)
[#351](#351)
* range() with single-argument
([#452](#452))
([d05f369](d05f369))
* Skip checking of redefined functions
([#457](#457))
([7f9ad32](7f9ad32))
* Support `nat`/`int` ↔ `bool` cast operations
([#459](#459))
([3b778c3](3b778c3))
* Use `hugr-cli` for validation
([#455](#455))
([1d0667b](1d0667b))
* Use cell name instead of file for notebook errors
([#382](#382))
([d542601](d542601))
* Use the hugr builder
([536abf9](536abf9))


### Bug Fixes

* Fix and update demo notebook
([#376](#376))
([23b2a15](23b2a15))
* Fix linearity checking bug
([#441](#441))
([0b8ea21](0b8ea21))
* Fix struct definitions in notebooks
([#374](#374))
([b009465](b009465))


### Documentation

* Update readme, `cargo build` instead of `--extra validation`
([#471](#471))
([c2a4c86](c2a4c86))


### Miscellaneous Chores

* Update hugr to `0.8.0`
([#454](#454))
([b02e0d0](b02e0d0))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants