Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bamarsha
Copy link
Contributor

@bamarsha bamarsha commented Sep 11, 2020

The Capability attribute is used by the capability inference feature (see microsoft/qsharp-compiler#600), which automatically adds the attribute to callables when a library is compiled. The attribute can also be used to override the inferred capability.

The value of the attribute is the name of the capability level, which means the names will be part of the public Q# API. However, the current names "Unknown," "QPRGen1," and "QPRGen0" are temporary, so we need to finalize those before this PR should be merged.

Alternatively, the Capability attribute could use an Int instead of a String to avoid picking names, but I think that would be more cryptic - and because there are only a finite number of levels, a full Int doesn't seem right. String is also not quite right, but we don't have discriminated unions or enums yet.

We might also want to consider what happens if there are capabilities along multiple dimensions, where a single value is not enough to represent the required capabilities accurately.

TODO:

  • Rename Capability to RequiresCapability
  • Move the attribute out of the Microsoft.Quantum.Core namespace
  • Add a Reason : String item to the attribute
  • Finalize names of capability levels "Unknown," "QPRGen1," and "QPRGen0"

@alan-geller
Copy link

One nice thing about using a string is that you could use multiple appearances of this attribute to indicate multiple, additive capabilities in the multi-dimensional case. It's a bit harder with an integer. Strings are definitely more sensitive to typos -- a DU or enum would be nice to have for this -- but they're more flexible as well.

That said, I would try to avoid having many orthogonal capabilities if we can. It makes things much harder to understand than if there's a simple linear ordering. If we can't make the simple linear ordering work, then we'll deal with that, but I think it makes sense to start with linear capability levels and break that only if it's really necessary.

@bamarsha
Copy link
Contributor Author

bamarsha commented Oct 1, 2020

I had to add a capability override to Reset to fix an issue with IonQ thinking Reset used unsupported capabilities:

@Capability("QPRGen0")
operation Reset (target : Qubit) : Unit {
    if (M(target) == One) {
        X(target);
    }
}

Since @Capability("QPRGen0") looks kind of strange without context, I'm thinking of adding a required Reason : String property, something like:

@Capability("QPRGen0", "Reset is replaced by a supported operation on all execution targets.")
operation Reset (target : Qubit) : Unit {
    if (M(target) == One) {
        X(target);
    }
}

Copy link
Contributor

@cgranade cgranade left a comment

Choose a reason for hiding this comment

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

I realize this is still in draft, but just wanted to provide early feedback in the hopes that it would be helpful. If I could ask, if this new attribute is primarily intended to be emitted by compiler rewrite steps, would it make sense to move it out of Microsoft.Quantum.Core so as to not add noise to autocomplete suggestions for all Q# source files? Something like Microsoft.Quantum.Targeting, perhaps?

@bamarsha
Copy link
Contributor Author

bamarsha commented Oct 1, 2020

If I could ask, if this new attribute is primarily intended to be emitted by compiler rewrite steps, would it make sense to move it out of Microsoft.Quantum.Core so as to not add noise to autocomplete suggestions for all Q# source files? Something like Microsoft.Quantum.Targeting, perhaps?

That might work. It is intended to be used by users to override the compiler if needed, but that should happen rarely, since ideally the compiler will infer the right capability almost all of the time.

@anpaz
Copy link
Member

anpaz commented Oct 6, 2020

/AzurePipelines run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@anpaz
Copy link
Member

anpaz commented Oct 6, 2020

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@bamarsha bamarsha added this to the October 2020 milestone Oct 7, 2020
@bamarsha
Copy link
Contributor Author

bamarsha commented Oct 16, 2020

For reference, here's my proposed API of the attribute (which is different from the current code - wanted to get feedback before I change it here and in the compiler):

newtype RequiresCapability = (Level : String, Reason : String);

I am not sure what namespace to put it in - @cgranade mentioned Microsoft.Quantum.Targeting.

@bamarsha bamarsha marked this pull request as ready for review October 16, 2020 18:03
@bamarsha bamarsha changed the title Add Capability attribute to Microsoft.Quantum.Core Add RequiresCapability attribute Oct 19, 2020
@bamarsha bamarsha requested a review from cgranade October 19, 2020 22:27
Copy link
Contributor

@cgranade cgranade left a comment

Choose a reason for hiding this comment

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

Strictly speaking, this should go through Q# API review for approval at the start of next month's development cycle; that said, I think given that this is a very small change that's mainly focused on a fairly niche use case, I'm OK to approve directly in this PR (@msoeken and @guenp, do you concur?), modulo a few comments about API documentation.

@bamarsha
Copy link
Contributor Author

Strictly speaking, this should go through Q# API review for approval at the start of next month's development cycle; that said, I think given that this is a very small change that's mainly focused on a fairly niche use case, I'm OK to approve directly in this PR (@msoeken and @guenp, do you concur?), modulo a few comments about API documentation.

We could hold it until next month to go through API review. @bettinaheim Is there a reason to get it in this month?

@guenp
Copy link

guenp commented Oct 20, 2020

@SamarSha @cgranade I suggest to merge this and have a post-discussion in the API review meeting and in the (perhaps unlikely) event where we want to change anything we can do a follow-up PR

bamarsha and others added 2 commits October 20, 2020 12:08
Co-authored-by: Chris Granade <chgranad@microsoft.com>
@bettinaheim
Copy link
Contributor

@bettinaheim Is there a reason to get it in this month?

@SamarSha We are good to pull that in as per Guen's suggestion and having talked to Chris. Please go ahead with merging this.

@github-actions github-actions bot merged commit 2325f8c into main Oct 21, 2020
@github-actions github-actions bot deleted the samarsha/capability-attribute branch October 21, 2020 03:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants