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

Load Katas package automatically at startup#468

Merged
tcNickolas merged 65 commits intomainfrom
rmshaffer/package-references
Oct 6, 2020
Merged

Load Katas package automatically at startup#468
tcNickolas merged 65 commits intomainfrom
rmshaffer/package-references

Conversation

@rmshaffer
Copy link
Contributor

@rmshaffer rmshaffer commented Aug 26, 2020

This PR contains the following changes:

  • Adds the IQSharpLoadAutomatically property to each .csproj that will cause IQ# to load the listed packages at initialization time.
  • Removes the %package Microsoft.Quantum.Katas and %workspace reload calls from each notebook, since those are no longer necessary with the above change.

To do:

  • Ensure CI passes reliably.
  • Update to released QDK version.

@rmshaffer rmshaffer changed the title Replace project references with package references, update to 0.12.20082513 Load Katas package automatically at startup, update QDK to 0.12.20082513 Aug 26, 2020
@rmshaffer rmshaffer changed the title Load Katas package automatically at startup, update QDK to 0.12.20082513 Load Katas package automatically at startup Aug 26, 2020
@rmshaffer rmshaffer changed the base branch from master to rmshaffer/csproj-package-references August 27, 2020 15:36
Base automatically changed from rmshaffer/csproj-package-references to master August 27, 2020 21:43
@tcNickolas
Copy link
Contributor

Sounds good - I'm fine with a minute-long kernel startup time, since I don't imagine the users need to run the notebook and run the cells immediately, I hope they take some time to actually read the tasks :-) But a message like this is going to look pretty bad, especially in the events we're doing in October (for a regular user as well, but getting a message like this during a demo is even worse)

@rmshaffer
Copy link
Contributor Author

In the PR at microsoft/iqsharp#331, I have changes which should resolve this issue. Specifically, the behavior will be that the kernel reports itself as ready "immediately" (i.e., same as prior behavior). The package load and workspace compilation will then continue in the background.

If the user immediately tries to execute the first cell, it will simply take a while to execute (because it will wait for the all of the initialization to finish first). To the user it just looks like the cell is taking some time to execute - no error message, as Jupyter doesn't apply any timeouts to cell execution.

If the user reads instructions for a while before executing the first cell, then the first cell execution should be fast, just as if today they had already invoked the %package Microsoft.Quantum.Katas and %workspace reload cells.

@tcNickolas
Copy link
Contributor

Sounds reasonable - the user who's going to be typing in the solution and running it right away is most likely to be me, I expect others to take a bit more time :-)

@rmshaffer rmshaffer marked this pull request as ready for review October 5, 2020 15:15
@rmshaffer
Copy link
Contributor Author

@tcNickolas, I believe this is ready to go now, finally. Please let me know if you have any concerns, or if we can go ahead and merge this!

@tcNickolas
Copy link
Contributor

Can you give a quick summary of what this change ended up being? Also, in some places the new version is 0.12.20100301 and in some 0.12.20092803, can you please unify that?

@rmshaffer
Copy link
Contributor Author

Can you give a quick summary of what this change ended up being?

I think the PR description is still accurate:

  • Adds the IQSharpLoadAutomatically property to each .csproj that will cause IQ# to load the listed packages at initialization time.
  • Removes the %package Microsoft.Quantum.Katas and %workspace reload calls from each notebook, since those are no longer necessary with the above change.

At the moment, this PR also includes the QDK version update to 0.12.20100301. If you'd prefer, we can do that separately. PR #494 is ready to go and includes only the QDK version update. We could merge that one first, then follow it up with this one.

Also, in some places the new version is 0.12.20100301 and in some 0.12.20092803, can you please unify that?

Sure, looks like only a couple of comments still referenced 0.12.20092803 - I've updated those.

Copy link
Contributor

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

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

Looks good, and I did some local validations to verify that things work as expected. Thank you!

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.

2 participants