Attempt to Fix PauliSumCollector.collect_async warning under notebook(short-term solution) #4004
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Oh, I didn't realize this value was incorrect - thanks for this. I was mistakenly content with CI passing after a long battle, mea culpa. (Note the papermill error was totally unrelated.) |
| @@ -789,7 +803,7 @@ | |||
| "collector = cirq.PauliSumCollector(circuit, psum, samples_per_term=10_000)\n", | |||
| "\n", | |||
| "# Provide a sampler. See also: collector.collect(...).\n", | |||
| "collector.collect_async(sampler=cirq.Simulator())\n", | |||
| "collector.collect(sampler=cirq.Simulator())\n", | |||
There was a problem hiding this comment.
I tried to run this cell, but it's taking an unreasonably long time to complete (5+ minutes). To minimize the impact on test times, we should reduce samples_per_term to a number that allows this to complete in 30 seconds or less.
If this requires choosing an unrealistically low value for samples_per_term, please also add a comment explaining why we're using the new value.
There was a problem hiding this comment.
That is strange. It is super quick to run that. In google colab that line takes less than 300ms.
In the github action, the whole notebook execution is like one and half minutes.
I wonder why it takes you so long time to complete. (I guess maybe related to live lock?) Could you show your test environment?
There was a problem hiding this comment.
I'm running this directly from the Colab link (modified to point to the latest commit on this PR) and the execution consistently hangs or crashes - I haven't had a run complete successfully yet.
Logs include IndexError: pop from an empty deque, but it's unclear whether that's coming from the Colab restart or from asyncio.
There was a problem hiding this comment.
For clarity, I've been running the linked colab with the Runtime > Run all command.
There was a problem hiding this comment.
Interesting. Yes, if I click Run all, it will fail. But if click Run before that Cell and then manually shift+Enter run that cell can be finished quickly. I guess Run all added another layer of running environment caused this. I will take a look to understand this better.
There was a problem hiding this comment.
Interesting. Yes, if I click
Run all, it will fail. But if clickRun beforethat Cell and then manuallyshift+Enterrun that cell can be finished quickly.
I can reproduce this result - Run before and then running this cell also succeeds for me. Thanks for digging into this!
There was a problem hiding this comment.
I still do not understand why this happens. What I found is the TensorFlow Federated colab has encountered similar asynchrony issue and they used nest_asyncio as well for solution. I tried to use Run all their colab, and they do not have similar issue :/
|
@ybc1991, have you been able to find any solution for this? Based on #4197 it appears that the issue is still occurring. If we have no way to resolve this from our end (e.g. if it's caused by an issue with nest_asyncio that has no workaround), I'd consider moving forwards with the PR with clear warning messages on the affected cell noting that "Run all" will misbehave. |
I don't find the solution to fix it. I do test on other environment such as local jupyter notebook and the note book in vscode. Both can finish correctly with run-all button. So as suggested, I added a warning cell there. PTAL. |
docs/operators_and_observables.ipynb
Outdated
| "id": "5d8e48230d73" | ||
| }, | ||
| "source": [ | ||
| "**Warning: If you run this notebook in colab environment with \"Run all\", the process may hang forever or you may encounter crash. Please manually run this cell either press the play button to the left of the code, or use the keyboard shortcut \"Command/Ctrl+Enter\".**" |
There was a problem hiding this comment.
A few nits:
in colab environment->in a colab environmenteither press...or use->either by pressing...or using- Instead of
the process, I recommend sayingthe cell belowto be abundantly clear which cell is the source of the issue. (As it is, it's unclear if this refers to the cell above or below)
There was a problem hiding this comment.
Thanks for this. Would you mind either commenting on #4004 or opening a new issue to track the "run all" dilemma here? I'm happy to merge this PR as-is, I just want to keep tabs on this for the future.
BichengYing
changed the title
|
Sure, I have updated the title and content of this PR. To point out this is the short-term solution and the problem of "run all" in the top comments. |
95-martin-orion
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
|
Automerge cancelled: A status check is failing. |
CirqBot
removed
automerge
|
The error is an instance of #4293, unrelated to this PR. |
I reran the checks, that seems to push the situation further. This is a nasty flake from npm, as I'm looking into it, retries might be the only way to get over it until they fix it upstream. |
balopat
added
the
automerge
|
Automerge cancelled: A status check is failing. |
CirqBot
removed
the
automerge
…(short-term solution) (quantumlib#4004) * add nest_asyncio to solve collector under notebook environment * Fix the typo in clifford_gate::test_commutes_pauli * Revert "Fix the typo in clifford_gate::test_commutes_pauli" This reverts commit b2d9269. * Add warning above the cell * Fix for nit Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>
As mentioned in #3654:
The current status is using collect_async. However, in the website, it shows the warning like:
Also, the Energy 0.0 is not desired value, which is because the collector is not finished.
The problem mainly is because notebook itself is running under the asyncio. Importing
nest-asynciomaybe is the easist fix.Note:
nest-asyncioseems working well in most cases, but in the colab environment,run allcan cause the cell of async function hang forever. Fortunately, it can be avoided by run cell one-by-one.