feat!: replace opaque type arguments with String #1328

ss2165 · 2024-07-19T15:47:40Z

BREAKING CHANGE: opaque type parameters replaced with string parameters.

codecov · 2024-07-19T15:55:06Z

Codecov Report

Attention: Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.48%. Comparing base (dbb3232) to head (92d5c0a).

Files	Patch %	Lines
hugr-core/src/types/type_param.rs	50.00%	4 Missing ⚠️
hugr-py/src/hugr/serialization/tys.py	88.88%	1 Missing ⚠️
hugr-py/src/hugr/tys.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1328      +/-   ##
==========================================
+ Coverage   87.38%   87.48%   +0.10%     
==========================================
  Files         111      111              
  Lines       19853    19787      -66     
  Branches    17586    17524      -62     
==========================================
- Hits        17348    17311      -37     
+ Misses       1729     1702      -27     
+ Partials      776      774       -2

Flag	Coverage Δ
python	`91.51% <88.23%> (+0.07%)`	⬆️
rust	`86.96% <66.66%> (+0.10%)`	⬆️

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.

ss2165 · 2024-07-22T09:57:48Z

Converting to draft as along the lines of: #1308 (comment)
I'm not sure this is worth merging in this state.

acl-cqc

Hmmm, so I can see some good points here, but I wonder if we have gone too far in some respects....and not far enough in others ;-)

If I understand correctly, the "value" can be arbitrary bytes, not necessarily valid UTF-8. But, when we serialize, we base64-encode, and then write that out as "utf-8" (i.e. ASCII, there are no unicode characters in base64!)
- And, printing the byte array will fail (!) if it's not valid UTF-8.
- I think here we might want to go further. Particularly given second point (below). When we are dealing with a Hugr (potentially containing "blackbox" ops and/or types that we don't understand), we'll be seeing ports (potentially) with types like "SomeType". That is, the type contains a byte array value. Printing this out is the thing we need to do here, and I think it'll help a lot if it prints out in a vaguely-human-readable format (or any format, rather than not at all). Why not require the value to be a string (i.e. valid unicode), indeed maybe even base64-encoded, already.
Secondly, the "untyped" bit. I think we've gone back-and-forth between CustomTypeArgs being "checkable" (type provided from outside) vs "synthesizable" (know their own type) a few times. Here we've very much switched to the former.
- If you have the extension loaded, then you always can look up the declaration (of the TypeParam for which the TypeArg is a value) to retrieve the type of which the value is an instance. You can, but it's much more awkward (in code) to do than if the TypeArg just provides the type.
- If you don't have the extension then you're even more stuffed (in practice this shouldn't be an issue as type declarations are easily passed around, right)

So I'd go with (a) keep the type in the CustomTypeArg; (b) make the extension serialize to at least string rather than bytes. I think that still achieves the critical goal of making the extension determine the serialization (and thus fixing roundtrip)....?

acl-cqc · 2024-07-22T09:08:55Z

hugr-core/src/extension/declarative.rs

  operations:
  - name: UnsupportedOperation
    description: An operation from 3 qubits to 3 qubits
-    params:
+    # params:


This seems odd - presumably, the "are not supported" is meant to refer to declarative.rs rather than extensions generally, and this commenting-out is a drive-by?

as of this PR only opaque type parameters are supported, whereas these examples include TypeTypeParam etc. (which were previously being treated incorrectly as opaque). So commenting out as fixing that isn't in scope of this PR.

acl-cqc · 2024-07-22T11:15:08Z

Also, I don't think this fixes #1308. I agree it fixes #1048, which is the blocking/urgent bug, so it might be worth taking it for that, but keeping #1308 open. However I'd not let this go in as a fix for #1308 until discussion there has resolved.

Closes #1308 - Previously held a yaml value, this doesn't really make much sense - If it is an opaque blob, no reason it needs a HUGR custom type attached to it? - Now truly opaque, it is up to extension-aware tooling to use the bytes data as they see fit - Serialised as base64 encoded string - Utilities for common case, strings - Conducive with adding typed "operation intrinsics/properties" as a follow-up. This would be hugr-typed values that can be attached to operations (in a semantically significant manner, unlike metadata) but perhaps are not allowed to affect the signature. - should there be a cap on size of byte array? If so should we use `[u8; N]` over `Vec<u8>`? - Serialisation break to be handled TODO BREAKING_CHANGE: opaque type arguments are now untyped and hold bytes rather than a yaml value. Serialised form is base64 string.

acl-cqc

Looks good, thanks. Are you sure you don't want to rename hugr_py tys.OpaqueParam? Nothing major apart from that.

acl-cqc · 2024-07-24T14:02:57Z

hugr-core/src/extension/declarative/types.rs

@@ -100,14 +100,11 @@ impl From<TypeDefBoundDeclaration> for TypeDefBound {

 /// A declarative type parameter definition.
 ///
-/// Either a type, or a 2-element list containing a human-readable name and a type id.
+/// Only supports opaque parameters for now.


Suggested change

/// Only supports opaque parameters for now.

/// Only supports string parameters for now.

Or even better:

Suggested change

/// Only supports opaque parameters for now.

/// Only supports [TypeParam::String]s for now.

acl-cqc · 2024-07-24T14:03:11Z

hugr-core/src/extension/declarative/types.rs

-    Type(TypeName),
-    /// A 2-tuple containing a human-readable name and a type id.
-    WithDescription(String, TypeName),
+    Opaque,


Rename this to String also

acl-cqc · 2024-07-24T14:04:06Z

hugr-core/src/extension/declarative/types.rs

@@ -119,52 +116,9 @@ impl TypeParamDeclaration {
    /// TODO: The parameter description is currently ignored.


The parameter description no longer exists, so drop the comment.

Or reinstate the description - it's harmless, could sit there as a reminder what the string is actually supposed to be, but I absolutely do not insist

acl-cqc · 2024-07-24T14:06:38Z

hugr-core/src/types/type_param.rs

-            TypeArg::Opaque {
-                arg: CustomTypeArg { typ, .. },
-            } => {
-                // The type must be equal to that declared (in a TypeParam) by the instantiated TypeDef,


Good to see this headscratcher gone

acl-cqc · 2024-07-24T14:09:56Z

hugr-py/src/hugr/tys.py

@@ -88,10 +88,8 @@ def to_serial(self) -> stys.BoundedNatParam:
 class OpaqueParam(TypeParam):
    """Opaque type parameter."""


Update the doc at least, even if you are deciding not to rename the class. (Do we really avoid breakage by leaving it as OpaqueParam?? It seems odd not to rename when you are renaming the corresponding OpaqueArg -> StringArg)

acl-cqc · 2024-07-24T14:11:59Z

hugr-py/src/hugr/serialization/tys.py


    def deserialize(self) -> tys.OpaqueParam:
-        return tys.OpaqueParam(ty=self.ty.deserialize())
+        return tys.OpaqueParam()


It does seem odd not to be returning something called tys.StringParam here. I won't 100% insist (if there are serious migration issues) but I will strongly encourage

## 🤖 New release * `hugr`: 0.9.1 -> 0.10.0 * `hugr-core`: 0.6.1 -> 0.7.0 * `hugr-passes`: 0.6.1 -> 0.6.2 * `hugr-cli`: 0.2.1 -> 0.3.0 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.10.0 (2024-07-26) ### Bug Fixes (#1350)) - [**breaking**] Bump serialisation version with no upgrade path ([#1352](#1352)) ### Features - Add `nonlocal_edges` and `ensure_no_nonlocal_edges` ([#1345](#1345)) - Serialization upgrade path ([#1327](#1327)) - [**breaking**] Replace opaque type arguments with String ([#1328](#1328)) - Add `impl Hash for Type` ([#1347](#1347)) - `HasDef` and `HasConcrete` traits for def/concrete op design pattern ([#1336](#1336)) - Add pointer standard extension ([#1337](#1337)) - [**breaking**] Remove the `Eq` type bound. ([#1364](#1364)) ### Refactor - [**breaking**] Use JSON rather than YAML in opaque fields. ([#1338](#1338)) - [**breaking**] Declarative module behind optional feature flag ([#1341](#1341)) ### Testing - Miri gate serialization upgrades ([#1349](#1349)) </blockquote> ## `hugr-core` <blockquote> ## 0.7.0 (2024-07-26) ### Bug Fixes (#1350)) - [**breaking**] Bump serialisation version with no upgrade path ([#1352](#1352)) ### Features - Serialization upgrade path ([#1327](#1327)) - [**breaking**] Replace opaque type arguments with String ([#1328](#1328)) - Add `impl Hash for Type` ([#1347](#1347)) - `HasDef` and `HasConcrete` traits for def/concrete op design pattern ([#1336](#1336)) - Add pointer standard extension ([#1337](#1337)) - [**breaking**] Remove the `Eq` type bound. ([#1364](#1364)) ### Refactor - [**breaking**] Use JSON rather than YAML in opaque fields. ([#1338](#1338)) - [**breaking**] Declarative module behind optional feature flag ([#1341](#1341)) ### Testing - Miri gate serialization upgrades ([#1349](#1349)) </blockquote> ## `hugr-passes` <blockquote> ## 0.6.2 (2024-07-26) ### Features - Add `nonlocal_edges` and `ensure_no_nonlocal_edges` ([#1345](#1345)) </blockquote> ## `hugr-cli` <blockquote> ## 0.3.0 (2024-07-26) ### Features - [**breaking**] Created `validate` CLI subcommand. ([#1312](#1312)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Douglas Wilson <douglas.wilson@quantinuum.com>

🤖 I have created a release *beep* *boop* --- ## [0.5.0](hugr-py-v0.4.0...hugr-py-v0.5.0) (2024-07-29) ### ⚠ BREAKING CHANGES * Eq type bound removed. References to `Eq` in serialized HUGRs will be treated as `Copyable`. * **hugr-core:** All Hugrs serialised with earlier versions will fail to deserialise * opaque type parameters replaced with string parameters. ### Features * **hugr-py:** `AsCustomOp` protocol for user-defined custom op types. ([#1290](#1290)) ([1db43eb](1db43eb)) * remove the `Eq` type bound. ([#1364](#1364)) ([1218d21](1218d21)) * replace opaque type arguments with String ([#1328](#1328)) ([24b2217](24b2217)), closes [#1308](#1308) * Serialization upgrade path ([#1327](#1327)) ([d493139](d493139)) ### Bug Fixes * add op's extension to signature check in `resolve_opaque_op` ([#1317](#1317)) ([01da7ba](01da7ba)) * **hugr-core:** bump serialisation version with no upgrade path ([#1352](#1352)) ([657cbb0](657cbb0)) * **hugr-py:** ops require their own extensions ([#1303](#1303)) ([026bfcb](026bfcb)), closes [#1301](#1301) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

ss2165 requested a review from acl-cqc July 19, 2024 15:47

ss2165 requested a review from a team as a code owner July 19, 2024 15:47

ss2165 force-pushed the ss/custom-typearg branch 2 times, most recently from 552047c to cf6a425 Compare July 19, 2024 16:00

ss2165 marked this pull request as draft July 22, 2024 09:57

acl-cqc reviewed Jul 22, 2024

View reviewed changes

ss2165 added 2 commits July 24, 2024 12:35

replace opaque bytes with actual string

916a80b

ss2165 force-pushed the ss/custom-typearg branch from cf6a425 to 916a80b Compare July 24, 2024 13:50

ss2165 changed the title ~~feat!: opaque type arguments are untyped bytes.~~ feat!: replace opaque type arguments with String Jul 24, 2024

remove CustomTypeArg and cleanup

32fb093

ss2165 requested a review from acl-cqc July 24, 2024 13:54

ss2165 marked this pull request as ready for review July 24, 2024 13:54

acl-cqc approved these changes Jul 24, 2024

View reviewed changes

review comments

92d5c0a

ss2165 enabled auto-merge July 24, 2024 14:41

ss2165 added this pull request to the merge queue Jul 24, 2024

Merged via the queue into main with commit 24b2217 Jul 24, 2024
21 of 22 checks passed

ss2165 deleted the ss/custom-typearg branch July 24, 2024 14:48

This was referenced Jul 24, 2024

chore(main): release hugr-py 0.5.0 #1291

Merged

chore: release #1330

Merged

This was referenced Jul 24, 2024

Allow Extensions to provide binary code to validate values #483

Closed

Add function to validate a TypeParam #624

Closed

All ops and types need to belong to a loaded extension declaration. #1344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: replace opaque type arguments with String #1328

feat!: replace opaque type arguments with String #1328

ss2165 commented Jul 19, 2024 •

edited

Loading

codecov bot commented Jul 19, 2024 •

edited

Loading

ss2165 commented Jul 22, 2024

acl-cqc left a comment

acl-cqc Jul 22, 2024

ss2165 Jul 22, 2024

acl-cqc commented Jul 22, 2024

acl-cqc left a comment

acl-cqc Jul 24, 2024

acl-cqc Jul 24, 2024

acl-cqc Jul 24, 2024

acl-cqc Jul 24, 2024

acl-cqc Jul 24, 2024

acl-cqc Jul 24, 2024

	/// Only supports opaque parameters for now.
	/// Only supports string parameters for now.

	/// Only supports opaque parameters for now.
	/// Only supports [TypeParam::String]s for now.

		@@ -119,52 +116,9 @@ impl TypeParamDeclaration {
		/// TODO: The parameter description is currently ignored.

		@@ -88,10 +88,8 @@ def to_serial(self) -> stys.BoundedNatParam:
		class OpaqueParam(TypeParam):
		"""Opaque type parameter."""

feat!: replace opaque type arguments with String #1328

feat!: replace opaque type arguments with String #1328

Conversation

ss2165 commented Jul 19, 2024 • edited Loading

codecov bot commented Jul 19, 2024 • edited Loading

Codecov Report

ss2165 commented Jul 22, 2024

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jul 22, 2024

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ss2165 commented Jul 19, 2024 •

edited

Loading

codecov bot commented Jul 19, 2024 •

edited

Loading