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

Add a blanket implementation for CustomSigFn #259

Merged
merged 5 commits into from
Jul 12, 2023
Merged

Conversation

aborgna-q
Copy link
Collaborator

  • Adds a blanket CustomSignatureFunction implementation for closures, for we don't want to add a description.
  • Drops the name and misc parameters, since they can be taken directly when creating the function along with the OpDef.

So we can just do:

let name = "MyOp".into();
let description = "".into();
let args = vec![];
let mut misc = HashMap::new();

let sig: Signature = todo!(); // Some complex computation
let signature = move |_args: &[TypeArg]| {
    Ok((sig.input.clone(), sig.output.clone(), ResourceSet::new()))
};

OpDef::new_with_custom_sig(name, description, args, misc, signature)

@aborgna-q aborgna-q requested a review from acl-cqc July 11, 2023 13:25
@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 11, 2023

  • Adds a blanket CustomSignatureFunction implementation for closures, for we don't want to add a description.

Yes, very neat

  • Drops the name and misc parameters, since they can be taken directly when creating the function along with the OpDef.

Ideally not - can you just make the blanket CustomSigFunc impl take and ignore those parameters?

My thinking is that if you might not be using a lambda - rather, that misc could be extra data that defines the op. So this would give yet another way of defining TKET1 ops: you have a Resource that has an OpDef for each different TKET1 op, but they all use the same CustomSigFunc which expects a serialized TKET1 op in the misc data.

(Thus: most of the specification is in the YAML, including the misc data and serialized TKET1 ops; the "self-registration" code just has to register the same CustomSigFunc for everything)

@aborgna-q
Copy link
Collaborator Author

Since OpDef always creates dyn CustomSignatureFunctions we can always do that specialization at initialization time.
In the case you mention, you can just create a TKET1CustomSigFn::new(name, misc) when creating the OpDef since you already have the exact same data there.

Doing only for the blanket impl with reduced parameters is a bit annoying. We can only define a single blanket impl (because Fn is a trait, so multiple Fns can clash) so it makes more sense to have it match the signature of compute_signature.

@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 11, 2023

I think that same argument says we should also remove misc from try_lower and then we might as well drop misc altogether? (If it's only for ops internal to the toolchain, rather than 3rd-party ops, then there's no need for it to go in the YAML!). That is, I think misc is useful so that a resource implementor can define the difference between ops in the YAML.

I don't see a problem with a CustomSigFunc impl only for Functions taking just the TypeArgs, note. (Sure, you could have variations, but I'm willing to bet that that's a common case.)

@aborgna-q
Copy link
Collaborator Author

I re-added the parameters in the trait method, but left them out in the blanket impl on closures as suggested.

@aborgna-q aborgna-q requested review from acl-cqc and removed request for acl-cqc July 12, 2023 12:26
Copy link
Contributor

@acl-cqc acl-cqc left a comment

Choose a reason for hiding this comment

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

Sweet, thanks @aborgna-q.

FWIW I did try to allow a Fn directly in CustomSignatureFunc but didn't manage to get it to compile - this is a very nice way to get that working :)

@aborgna-q aborgna-q merged commit f196546 into main Jul 12, 2023
6 checks passed
@aborgna-q aborgna-q deleted the feat/customSigFn-impl branch July 12, 2023 14:11
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.

2 participants