Using the @test command to trigger tests

[vivanwin]

This is a first commit to change the current way of testing, to a newer method which will use the Q# @test functionality.
There is also an update to the update script. This will make sure that the text which explains how to install IQsharp uses the latest version in the example.

[tcNickolas]

It is my understanding that #468 is going to remove most, if not all, instructions on updating IQ# to a specific version in the notebooks, so a part of this change might become obsolete. @rmshaffer, could you share your plan for #468?

[rmshaffer]

It is my understanding that #468 is going to remove most, if not all, instructions on updating IQ# to a specific version in the notebooks, so a part of this change might become obsolete. @rmshaffer, could you share your plan for #468?

I believe @vivanwin's approach here is better than the one I had started in #468. There is still a possibility that version mismatches could cause problems, since there is a version of the Katas package specified in the .csproj that may not match the installed version of IQ#. We could discuss further, but I'm inclined to change my PR to follow the pattern proposed here.

[rmshaffer]

Looks good to me! (Pending CI issues, of course -- I can help investigate those if needed.) Will leave for @tcNickolas to approve.

[rmshaffer]

Do you want to remove the "0.1.2.3" from here as well?

[vivanwin]

I have updated this to include the actual version.

[rmshaffer]

This is unrelated to your change, but I just noticed that these instructions are not correct for users who have installed IQ# using Anaconda. For those users, the command would be just one line:

conda update -c quantum-engineering qsharp==0.12.20082513

@tcNickolas do you think it is worth updating all of the notebooks to include this instruction as well, since conda is now the recommended installation path for IQ#? I would be happy to do that in a future PR.

[vivanwin]

I think it could not hurt to add this.

[tcNickolas]

I really liked the part of your PR in which the installation instructions were removed from the notebooks altogether together with the %package and %workspace reload cells. Ideally there should be one place for installation instructions, and that's the main project README; I don't really like the idea of covering all possible scenarios of installation in each notebook.

[rmshaffer]

@tcNickolas yes, I like that part of my PR too. 😊

If you think it's appropriate, then, I like the idea of moving the installation instructions to the main README. I do believe the likelihood that users run into problems here is reduced with recent changes to versioning and package loading.

One idea: Put the installation instructions only in the main README, and then in each notebook, have a small blurb of text that says something like: "Getting errors? See the README."

[rmshaffer]

@vivanwin I believe I've identified the root cause of the CI failures here. It appears to be a bug in IQ# -- the kernel is reporting itself as ready to execute cells too early, before the packages have finished loading. (It's a timing issue, and unfortunately the CI tries to execute the first cell immediately after the kernel reports itself as ready.) There's a simple fix, but unfortunately it's in IQ# code and wouldn't be available until the next release.

Since it's a timing issue, I am hoping this can be worked around for now by keeping the %package Microsoft.Quantum.Katas::0.12.20082513 and/or the %workspace reload commands in the notebook, so that the packages have time to finish loading before the first %check_kata command is executed. Can you try that?

[tcNickolas]

If this is indeed the case, it's unfortunate, I was looking forward to getting rid of those cells :-(

Here is what I would do in this case:

1. Do the @Test change alone, modifying the projects but not the notebooks. Since the master is already on the latest IQ# version and our update instructions ask to have the installed IQ# version match the notebook version, the instructions should be fine (there is a silver lining to the fact that we never got to updating the update instructions to suggest only matching 0.12 part of the version!)
2. Separately, double-check the installation instructions in the main README to see whether they reflect our latest state. At a glance, this section lists packages that individual tutorials need installed, and it should include numpy and matplotlib for QuantumClassification tutorial.
3. (optional) Separately, update all of the per-notebook update instructions to include conda information. I'll admit I'm a bit reluctant to do that, since these cells will go away in less than a month, but for the sake of completeness we can do that.
4. (at some point between now and the next QDK release) I think it's time to create a troubleshooting guide for the katas: if you have error X, it's probably reason Y.
5. (after the next QDK release) Finish the change removing the %package and %workspace cells, together with the per-notebooks update instructions, and replace them with a link to the troubleshooting guide.

What do you think?

[cgranade]

@vivanwin I believe I've identified the root cause of the CI failures here. It appears to be a bug in IQ# -- the kernel is reporting itself as ready to execute cells too early, before the packages have finished loading. (It's a timing issue, and unfortunately the CI tries to execute the first cell immediately after the kernel reports itself as ready.) There's a simple fix, but unfortunately it's in IQ# code and wouldn't be available until the next release.

Since it's a timing issue, I am hoping this can be worked around for now by keeping the %package Microsoft.Quantum.Katas::0.12.20082513 and/or the %workspace reload commands in the notebook, so that the packages have time to finish loading before the first %check_kata command is executed. Can you try that?

Would the same issue affect the IQSHARP_AUTO_LOAD_PACKAGES environment variable?

[rmshaffer]

Would the same issue affect the IQSHARP_AUTO_LOAD_PACKAGES environment variable?

That's a great point, @cgranade! I hadn't thought of that. That code path is separate and should not be affected by this issue.

@vivanwin, to try this out:

1. Re-apply your notebook changes. (Sorry!)
2. Add the following line to validate-notebooks.ps1, just before the call to jupyter nbconvert:

   $env:IQSHARP_AUTO_LOAD_PACKAGES = "Microsoft.Quantum.Standard,Microsoft.Quantum.Katas,Microsoft.Quantum.Xunit"
   

That should ensure that the listed packages have all been loaded before executing the first cell.

In my testing this causes the kernel startup to take a bit longer (as would be expected, since it's loading additional packages), and so in order for CI to pass reliably you may also want to increase the startup timeout by adding something like:
--ExecutePreprocessor.startup_timeout=180
to the jupyter nbconvert command.

[tcNickolas]

@rmshaffer Is this change necessary for adopting @test attribute? If not, I'd really prefer to keep it separate from this PR, as I suggested in this comment. I know it is really tempting to do all improvements at once, but they all have the potential to be breaking, so mixing them together makes troubleshooting that much harder. It's a better engineering practice to do unrelated changes separately, one at a time.

[rmshaffer]

@rmshaffer Is this change necessary for adopting @test attribute?

No, it's not. It's only necessary for removing the %package and %workspace reload cells.

Yes, agreed, we should do this change separately (and I will investigate that as part of my PR #468).

@vivanwin, please disregard my previous suggestion, I'll try that out myself! 😊

[tcNickolas]

Looks good - I tried it locally, and I love how this gives fewer levels of nesting for the tests in VS Test Explorer. Also, it's neat that we can use different simulators for different tests (for example, in task 1.2 we don't really need CounterSimulator, QuantumSimulator would be enough) and they don't look non-uniform.

Thank you! I'll merge this, and I think we can do a similar switch for the rest of the katas.