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

CustomTypeArg should hold a Value #1308

Closed
doug-q opened this issue Jul 16, 2024 · 6 comments · Fixed by #1328 or #1291
Closed

CustomTypeArg should hold a Value #1308

doug-q opened this issue Jul 16, 2024 · 6 comments · Fixed by #1328 or #1291
Assignees

Comments

@doug-q
Copy link
Collaborator

doug-q commented Jul 16, 2024

Currently CustomTypeArg holds a serde_yaml::Value payload. We want to change this to a serde_json::Value #1048 , I suggest instead we should change it to crate::ops::custom::Value

@ss2165 ss2165 self-assigned this Jul 17, 2024
@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 17, 2024

Discussed this quite a bit with @ss2165. Thoughts

  • CustomTypeArg is a bit of a backdoor. We may want it for Ops, but it seems a bit wild to parametrize Types by such. (Which is to say, we didn't think of any use cases for such a type).
  • Moreover, Eq is not really defined for custom types (CustomTypeArg claims to be Eq, but this is by comparing json/yaml serialization which is not a reliable roundtrip - and potentially produces lots of false negatives due to formatting). Indeed, CustomTypes specifically include float and we really don't want types parametrized by float!
  • TypeArg + TypeParam are a bit of a hybrid between what we might want for parametrizing Types, and for parametrizing Ops (we would not mind if the latter was more flexible, including dropping Eq and allowing float).

This leads to a design space including options like....

  1. Make CustomTypeArg contain a Value, but rule out CustomTypeArg as a TypeArg to Type(Def), and allow it only as an arg to an OpDef. (TypeArgs given to an OpDef can include Types, but TypeArg's inside those should not include CustomTypeArg either. So it's a bit like row variables, the top level is different, but reversed - CustomTypeArg is extra at the top level for OpDefs, whereas RowVariables are ruled out at the top level for nodes.)
    • This means the CustomTypeArg's are passed to compute_signature, which might be essential, but sounds a bit wild if not essential (and we think the latter?)
  2. Remove CustomTypeArg altogether; add a separate mechanism for an OpDef to have "static parameters" (NOT static type parameters) that are values. These have declared custom types (e.g. float for a static non-computed angle).
    • Preferably, they are not passed to compute_signature, but possibly compute_signature can return a specification of what static params the op should take.
    • Note however that if we want to be able to substitute in angles specified as post-compilation runtime parameters ("command-line arguments"), that means angle must be a node/ExternalOp fed in as a runtime param, not a "static parameter", so angle is not a good example here.
    • This is nicer: Type and TypeArg can be Eq (without cheating), Op and static-params are not. But, more change....

ss2165 added a commit that referenced this issue Jul 19, 2024
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.
ss2165 added a commit that referenced this issue Jul 19, 2024
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.
ss2165 added a commit that referenced this issue Jul 19, 2024
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.
@doug-q
Copy link
Collaborator Author

doug-q commented Jul 22, 2024

I mostly agree with Alan above, CustomTypeArgs are too powerful and should be weakened. The impl Eq is dodgy, and we should fix this. Indeed I want to add Hash #1329 .

I was of the view that it is important to retain the ability for compute_signature to depend on some arbitrary user defined data, but I have changed my mind. If you wanted to do that, you could always add a Param of FunctionType, or two List params that specified the signature.

I do think static Value parameters would be somewhat nicer.

Use case for compute_signature depending on arbitrary user defined data:

We will need to support qir -> hugr to run tket optimisations on qir. The simplest way I can see to do this is to define a llvm.swiss_army_knife op that takes some ad-hoc JSON

@ss2165
Copy link
Member

ss2165 commented Jul 22, 2024

I do think static Value parameters would be somewhat nicer.

I agree. Should we be attaching this to nodes or rely on our existing infrastructure for this - i.e. all ops can have static edges?

Another idea: add a value->static op that has to be removed at runtime, potentially gives us way more powerful constexpr handling?

(I think Zig does this by emulating the target but how we actually implement the evaluation is not important here I think)

@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 22, 2024

We will need to support qir -> hugr to run tket optimisations on qir. The simplest way I can see to do this is to define a llvm.swiss_army_knife op that takes some ad-hoc JSON

...and computes a type from it?

Yeah, ugh, I mean, we ought to have some backdoor way to do that - it does not necessarily have to be nice or easy...

For example, having to pass the desired FunctionType as well as the "ad-hoc JSON", would be fine, but I'd still want to be able to call extension code to validate that the FunctionType is correct wrt. the JSON, and since the compute_signature interface doesn't distinguish between validating and re-computing, you'd still just need to be calling that with both FunctionType and JSON....in which case you don't need to pass the former...

I love @ss2165's "take extra Static args" approach but that doesn't get any extra data into compute_signature either. If we really need that then either...we add extra args to compute_signature (somehow, with potentially-annoying breakage); add an extra validate_signature function that takes the type args as well as the static params (a bit ugh, I think, but more backwards-compatible); or pass the desired type explicitly in the TypeArgs and delay the point of validation until inside some extension-understanding tool. None of these seems great although I wouldn't say any were terrible either. Thoughts?

@ss2165
Copy link
Member

ss2165 commented Jul 24, 2024

Lots of nice follow ups from the discussion above, but for now I'm going with replacing OpaqueArg with a string, and that should be addressing the original issue.

ss2165 added a commit that referenced this issue Jul 24, 2024
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.
github-merge-queue bot pushed a commit that referenced this issue Jul 24, 2024
Closes #1308

BREAKING CHANGE: opaque type parameters replaced with string parameters.
@ss2165
Copy link
Member

ss2165 commented Jul 24, 2024

Discussion on static-edge parameters moved to #1342

github-merge-queue bot pushed a commit that referenced this issue Jul 29, 2024
🤖 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).
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