Sketch out resources + extension mechanism, update CustomType #186
Conversation
src/resource.rs
Outdated
| pub resource_reqs: ResourceSet, | ||
| name: ResourceId, | ||
| // Set of resource dependencies required by this resource. | ||
| // TODO I haven't seen where these are required yet. If they are, |
There was a problem hiding this comment.
What about the case where someone wants to define a resource in terms of types provided by another resource?
This doesn't necessarily require any knowledge of the implementation of the other resource - one could have an operation on types from another resource which is defined in terms of that resources operations
There was a problem hiding this comment.
Yes, indeed. I'm not quite sure what this should look like. It may fall under the heading of "namespacing", which is one of the unresolved issues in this PR. Good point, thanks :)
There was a problem hiding this comment.
I've kept this in as an "upper bound" of all signatures that might be returned by compute_signature functions, which can't be determined by inspection.
However, I'm not entirely sure how it's used. If I declare an operation that takes a MyType<Int,6> without the resource declaring the type MyType being around (even in YAML!), seems like I don't lose anything - it's just an early-warning that I the end-user-programmer may not be able to get very far. But the HUGR is still capable of passing such values around as serialized blobs, if I have operations that both create and consume those values. Really, the only information added by the "declaration" of the custom type is validity checking of the type arguments?!
There was a problem hiding this comment.
Yes, that's the way I was thinking about it - that we'd be checking the validity of type arguments specified in operations
| @@ -31,8 +32,6 @@ pub enum EdgeKind { | |||
| Const(ClassicType), | |||
| /// Explicitly enforce an ordering between nodes in a DDG. | |||
| StateOrder, | |||
| /// An edge specifying a resource set. | |||
| Resource(ResourceSet), | |||
…>Arc
Custom{Signature,Lower}Func require Send + Sync
|
This has been sitting here awhile and @ss2165 is now away, but perhaps if @croyzor could review for direction and @aborgna-q for rustiness then that would be reasonable grounds to merge? |
There was a problem hiding this comment.
👍
Most of these comments reduce to "please add more docs". There's a load of new {Module, External, Custom} {Type, Op, Def, Param, Arg} that may confuse a first time reader. Perhaps a short resume of the structure as a top-level comment in resource would help...
src/resource.rs
Outdated
| #[serde(skip)] | ||
| None, |
There was a problem hiding this comment.
Again just thinking of the YAML - if there's not a fixed hugr, which can be given (e.g. filename) in the YAML file, then either the Resource has binary code that self-registers the custom function, or there is no possibility of lowering.
No need to plumb error out as signature computed when ResourceOp constructed
Lots of gaps here, and many things are not as neat as they might be eventually. But various OpDef and CustomOp classes are removed and/or combined, as per the spec. Also update the spec re. "Int".
I haven't implemented the loading code here, beyond that CustomOp's should serialize only the names of the OpDefs to which they refer (the loading code is TODO atm 'coz I haven't figured out the necessary serde). A function
resolve_extension_opscan be called after deserializing (which produces just names) to find the actual OpDefs from a "registry" (a Map<String, Resource>, see #188)We have now ruled out Ops parameterized by float(s); these will have to be passed in as static inputs.
Other questions/follow-up issues...
compute_signaturefunctions to have registered themselves before the YAML is loaded, whereas try_lower implementations could be added after loading YAML - this is a bit inconsistent, but follows from that every OpDef much have some way of computing Signatures, but may not provide any way of lowering.