[Utility magics]`%kata` magic should use simulators from `@Test` attribute

[Manvi-Agrawal]

trafficstars

Fixes https://github.com/microsoft/QuantumKatas/issues/580

Design document Important sections

• Features Planned
• Changes to KataMagic and CheckKataMagic
• Refractoring Magics
• Testing changes locally

Complete document: doc-link

[Manvi-Agrawal]

@anjbur, I have done the changes as requested by you.

• Maybe I could remove the SetDisplay of the CheckKataMagic if the code there is not much important. It seems to me that we are catching warnings in the ReferenceImplementation(one of the scenarios maybe to check if any deprecated syntax is being used).

• Note : Some cells in the jupyter notebooks don't have a return statement, so the validation of such notebooks(11 of them) is failing now. I suppose we check for the basic syntax, thats why many notebooks have a default return statement. Dont know why they were passing previously. The previous CheckKataMagic was using ICompilerService and now it uses ISnippets. So, I have the following options

  1. Try unification with Compiler of the type ICompilerService.
  2. Add the return statements
  3. Change the return type of the operation. Largely valid in the case where the operation returns a Bool. We could change it to return a 0 or 1, and by default we would return a -1. Such a change would require care, because the changes may need to be propagated down to the other tasks as well.
  4. Exclude the cells from validation by using invalid_code tag.
  5. Use a hybrid of Approach 2, 3 and 4.

!!!PS:!!! This issue is resolved now. Kindly refer this comment

[tcNickolas]

It is important to understand what changes in the code caused changes in validation outcomes, before we make decisions such as excluding tasks from validation or modifying the code. This change should only add use of new simulators, but not break anything in the validation of the existing tasks or remove any functionality. %kata magic always gave errors if the code was invalid, but those were benign, they didn't fail notebook validation because notebooks are not validated using %kata. %check_kata magic shouldn't give errors for invalid code in the cell itself, since it doesn't use that code.

On Thu, May 20, 2021 at 10:23 PM Manvi-Agrawal @.***> wrote:

Following is the list of notebooks affected by the new change

• DeutschJozsaAlgorithm.ipynb
• DeutschJozsaAlgorithmTutorial.ipynb
• GroversAlgorithm.ipynb
• MagicSquareGame.ipynb
• Measurements.ipynb
• Oracles.ipynb
• PhaseEstimation.ipynb
• QEC_BitFlipCode.ipynb
• SingleQubitSystemMeasurements.ipynb
• SuperdenseCoding.ipynb
• TruthTables.ipynb

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/microsoft/QuantumKatas/pull/620#issuecomment-845662196, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNFAADPWIKEWALO3AM6AZ3TOXU55ANCNFSM44EM7LCA .

[Manvi-Agrawal]

@tcNickolas will look for the changes that caused this. Meanwhile, do we want to retain the Prototype Kata in #621?

[Manvi-Agrawal]

It is important to understand what changes in the code caused changes in validation outcomes, before we make decisions such as excluding tasks from validation or modifying the code.

Hi @tcNickolas @anjbur, I am able to promote code reuse by AbstractKataMagic now. The core functionality of KataMagic and CheckKataMagic is also retained. The changes which caused these errors was that KataMagic was essentially compiling the code by Snippets.Compile(code) and CheckKataMagic used Compiler.IdentifyElements(code) to semi-compile the code. I did some validation for the KataMagic and CheckKataMagic using BasicGates and Prototype notebook. I then ran a custom script(slightly modified the current validate-notebooks.ps1) to check the changes in KataMagic and CheckKataMagic locally for all notebooks that have %kata magic. The main point of difference was that userAnswer was of the type

• OperationInfo in KataMagic
• string in CheckKataMagic

I did the following changes:

• Changed the signature from AbstractKataMagic to AbstractKataMagic<T>.
• This generic type T, allows userAnswer to switch between string and OperationInfo with ease. Also, an important point here to note is that usually we were usually using the string form of userAnswer which I get from userAnswer.ToString(). Also, note that OperationInfo overrides the ToString() method to return userAnswer.FullName
• Also, the new FindReferenceImplementation uses the namespace of skeletonAnswer rather than test. This change is still valid because currently skeletonAnswer, referenceAnswer and the test are in the same namespace.

[tcNickolas]

Validating this functionality is an interesting challenge, related to retaining the prototype kata. The issue #580 went unnoticed for a very long time, until we realized that the test code that should be failing on QuantumSimulator was passing. So to validate that the magics pick up the right simulators we need to reproduce the situation in which we noticed that they don't.

To figure out the set of things that need to be validated, we need to

1. Check all simulators from the set {QuantumSimulator, CounterSimulator and ToffoliSimulator} and create tests that pass on each of the simulators but fail on others.
2. Run these tests on the simulators on which they should pass, and check that they pass indeed.
3. Run these tests on the simulators on which they shouldn't pass, and check that they fail indeed.
4. Optionally, create tests that should run on two out of three.

The tests in the prototype kata from #621 don't actually use any functionality of the simulators, so they will pass as long as any simulator picks them up. So the test kata needs to be improved to contain non-trivial tests.

• Tests that pass on CounterSimulator only should use counting operations such as GetOracleCallsCount and ResetOracleCallsCount - the other two simulators don't define them, so those tests should fail at runtime.
• Tests that pass on QuantumSimulator but not on ToffoliSimulator should use gates other than X, CNOT and controlled variants of those - a Hadamard will fail immediately, since it's not supported by ToffoliSimulator. (I think we won't be able to do a test that passes on QuantumSimulator but fails on CounterSimulator, since the latter offers all functionality of the former.)
• Tests that pass on ToffoliSimulator only should allocate and manipulate a lot of qubits, like a few hundred - full state simulator should run out of memory trying to do this, though I'm not sure how it will look in notebooks, most likely a timeout.

Does that sound reasonable? @anjbur, any other thoughts? I imagine those tests would have to be non-automated now, and hopefully we'll be able to automate them later.

[tcNickolas]

Also, I don't think we need to use ResourcesEstimator and QCTraceSimulator at this point - I can imagine tests that use ToffoliSimulator easily, but these two don't have any effect noticeable from Q# code. Any tests using them would be written in C# or Python, estimating some Q# code and analyzing the results outside of Q#.

[Manvi-Agrawal]

Hi @tcNickolas , I have done some changes to include non-trivial tests as suggested by you in this comment in #621.

I am a bit confused about this.

Run these tests on the simulators on which they shouldn't pass, and check that they fail indeed.

• Is this experiment meant to be done on the local machine or we need to include the tests which should fail and exclude them from validation.

Any help would be greatly appreciated.

[Manvi-Agrawal]

Hi @tcNickolas @anjbur, I just thought of slightly cleaner approach to refractor the magics from the one mentioned here.

I am thinking to get away with the generics by considering userAnswer as a string in both the magics.

On analysing the code, I found that KataMagic needed the userAnswer in operation format just to replace the last solution entered by the user. Hence, the process of obtaining userAnswer as OperationInfo could be moved from GetUserAnswer to a new function FindSolution in KataMagic of the following signature type :

protected virtual OperationInfo FindSolution(string userAnswer)

I would love to know your opinion on my new approach.

[Manvi-Agrawal]

Hi @anjbur, I have changed the code to remove the generics. Unfortunately, I can't do the change here due to the reason in the following comment.

[Manvi-Agrawal]

Hi @anjbur. I have now changed the return type of GetDeclaredCallables to IEnumerable<QsNamespaceElement> as per your suggestion

[Manvi-Agrawal]

Hi @anjbur, I have applied the suggestions as requested by you.

And as for the testing changes to magics as per @tcNickolas suggestions described here, I made an initial attempt in PR#621. I have a minor confusion regarding whether the failing tests should be the part of the Prototype kata or not. Currently, I have excluded them from the Prototype kata. In my local machine, the simulator specific tests pass only on the desired simulators and fail on others. Any guidance would be truly appreciated.

[tcNickolas]

I agree that testing both failing and passing tests automatically is tricky. I would still keep the failing tests in the test notebook - this way a person validating the change manually will have an easier time doing that. We can exclude the failing tests in this notebook from CI validation using the trick that you proposed https://github.com/microsoft/QuantumKatas/blob/main/.github/CONTRIBUTING.md#excluding-individual-tasks-from-validation, so that they will be only available to a manual tester.

For the sequence of merging this PR and #621:

• We'll probably want to finalize #621 in parallel with this PR, so that we can do validations in a unified way and make sure all scenarios are covered properly.
• First we'll merge this PR.
• Then we wait for the next alpha package coming from our official builds, so that we can update the NuGet packages used in #621 with that alpha version. This is not a user-facing kata, so we can keep it on an alpha version for a bit, but I prefer not to store locally generated NuGet packages in the repo itself.
• Then we merge #621, so that validations of this change become part of our CI.
• After the next QDK release we'll switch all the packages in the repo to it, making this change available to all katas.

@anjbur How does that sound? Am I missing any validation steps we'll need?

[Manvi-Agrawal]

Hi @tcNickolas thank you for giving the suggestions here. I am hoping to get the changes done by the end of week. I hope that wouldn't be a problem.

[tcNickolas]

Of course it's not a problem! I know it can be hard to spend time on side projects during the week - I'm juggling that myself :-)

[Manvi-Agrawal]

Hi @tcNickolas , fortunately I got time to do the changes in PR#621 as per the suggestions here. Will try my best on resolving comments.

[Manvi-Agrawal]

Hi @anjbur. Thanks a lot for your precious time while reviewing this PR. I have applied the code suggestions through the UI interface. I currently have limited bandwidth, so hopefully I should be able to take a look at other changes in October.

[anjbur]

Hi @Manvi-Agrawal! I wanted to check in on what your plans are for this PR and if you still planned to add the changes in this or next month. I'll be unavailable for the next two weeks, so I wanted to let you know that depending on the timing I won't be able to take a look until mid-November.

[Manvi-Agrawal]

No problem. I will push my changes after Mid November.

[anjbur]

Hi, @Manvi-Agrawal! I wanted to check in to see if you still planned to push these changes. If not, we can make the final round of edits to get everything merged.

[Manvi-Agrawal]

Hi @anjbur, apologies for keeping this PR on on hold. I think it would be good if you could do the final round of edits.

TLDR: I have done most of the changes requested by you in your final review locally. But I am not able to validate my changes locally after updating to the qdk version using the steps mentioned in contributing guide.

The steps in contributing guide were added by me with the guidance provided by Maria and Ryan. The steps I were working fine until one these following qdk versions 0.17/0.18/0.19. I don't remember which version was it exactly. I think it would be good if you could update those steps for future contributors.

Thank you for your support regarding the final edits and your precious time for PR reviews.

[Manvi-Agrawal]

Hi @anjbur, I finally got some time to get back on this after a really long time :-) I just tested on the latest QDK v0.25.218240 locally that Prototype kata is working as per Mariia's guidance using the steps mentioned in contributing guide.

Are there any more changes/validations to be done? Thanks @anjbur and @tcNickolas for your guidance on this PR. Looking forward for feedback regarding Prototype kata introduced in https://github.com/microsoft/QuantumKatas/pull/621.

[tcNickolas]

We will be migrating the Katas to the new QDK that will use a different infrastructure (see https://devblogs.microsoft.com/qsharp/introducing-the-azure-quantum-development-kit-preview/ for the announcement), including a much faster simulator, so we won't continue to improve the existing infrastructure of Q# notebooks magics.

Thank you for your work on this, and apologies for not being able to incorporate it in our code earlier!

[Manvi-Agrawal]

Thanks @tcNickolas and @anjbur for your valuable guidance.

Looking forward to the new kata experience. Just had a look at https://quantum.microsoft.com/en-us/experience/quantum-katas, and the experience looks way more polished :-)

For my curiosity are the new katas, closed-source or open-sourced?

[tcNickolas]

The source for the new katas is at https://github.com/microsoft/qsharp/tree/main/katas

[Manvi-Agrawal]

Thanks @tcNickolas for the link.