Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hard-coded sleep in run_circuits #7793

Merged
merged 8 commits into from
Apr 1, 2022

Conversation

renier
Copy link
Contributor

@renier renier commented Mar 18, 2022

Summary

Fixes #7792

Since all the places where the _safe_get_job_status happen in
a context where the intended wait/sleep time is available, we can
add the argument with the value to remove the hard-coded 5 seconds.

Details and comments

This is my first time with a PR in this area. Let me know about any doc udpates
and test updates I need to do.

@renier renier requested review from a team, manoelmarques and woodsp-ibm as code owners March 18, 2022 20:41
Fixes Qiskit#7792

Since all the places where the `_safe_get_job_status` happen in
a context where the intended wait/sleep time is available, we can
add the argument with the value to remove the hard-coded 5 seconds.
@renier renier force-pushed the fix-7792-hardcoded-sleep-run-circuits branch from 4f68140 to 0b51020 Compare March 18, 2022 20:44
@coveralls
Copy link

coveralls commented Mar 18, 2022

Pull Request Test Coverage Report for Build 2078260187

  • 1 of 3 (33.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.716%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/utils/run_circuits.py 1 3 33.33%
Totals Coverage Status
Change from base Build 2075078519: 0.0%
Covered Lines: 53990
Relevant Lines: 64492

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this up. You need to run black to fix the lint failures tox -eblack will do this for you. See: https://github.com/Qiskit/qiskit-terra/blob/main/CONTRIBUTING.md#style-and-lint for more details.

While I think having a quick test to cover setting the wait value is respected would be useful, but the QuantumInstance code isn't that well tested (which is why there are a lot of bugs like this in my experience). So to do it you'd either have to play a lot of games with mock to force the run_circuits code to do what you want to verify we're waiting the correct duration. Or just run a algorithm tweak the wait value on the QuantumInstance constructor and assume if the result is correct your wait parameter was respected. You would need to do this on aer as a backend since the other available backend in ci from BasicAer is not async and nothing will wait for it to return. So I think just manually testing that this works is probably sufficient in this case (mostly out of apathy).

As for documentation, a quick release note probably in the fixes section would be good to just say that the QuantumInstance used at the core of algorithms in qiskit.algorithms now respects the wait parameter for sleeps while waiting for submitted jobs to return. The process for release notes is documented here: https://github.com/Qiskit/qiskit-terra/blob/main/CONTRIBUTING.md#release-notes

@mtreinish mtreinish added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Mar 18, 2022
@manoelmarques
Copy link
Member

Shouldn't we also decrease the wait default value from 5 seconds ? The runtime is being changed to pass 0.2 seconds but why not change it right in the QuantumInstance init also ?

@renier
Copy link
Contributor Author

renier commented Mar 21, 2022

Because 5 seconds might still be a more sensible default if you are running the QuantumInstance from outside the Qiskit Runtime environment. In which case checking the job status is a different process that goes through an Internet API each time. In the Qiskit Runtime environment, that might be just checking a value in memory.

@renier
Copy link
Contributor Author

renier commented Mar 24, 2022

@mtreinish thanks. added the reno note.

@renier
Copy link
Contributor Author

renier commented Mar 24, 2022

@Cryoris thanks for the lint run 👍

Cryoris
Cryoris previously approved these changes Apr 1, 2022
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

I tested it manually and it works, thanks @renier!

@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Apr 1, 2022
@mergify mergify bot merged commit 51d1139 into Qiskit:main Apr 1, 2022
mergify bot pushed a commit that referenced this pull request Apr 1, 2022
* Fix hard-coded sleep in run_circuits

Fixes #7792

Since all the places where the `_safe_get_job_status` happen in
a context where the intended wait/sleep time is available, we can
add the argument with the value to remove the hard-coded 5 seconds.

* run black

* add reno fix note

* grammar

* revising note accuracy

* Update releasenotes/notes/fix-hard-coded-sleep-run-circuits-a1588164e61d5336.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 51d1139)
mergify bot added a commit that referenced this pull request Apr 1, 2022
* Fix hard-coded sleep in run_circuits

Fixes #7792

Since all the places where the `_safe_get_job_status` happen in
a context where the intended wait/sleep time is available, we can
add the argument with the value to remove the hard-coded 5 seconds.

* run black

* add reno fix note

* grammar

* revising note accuracy

* Update releasenotes/notes/fix-hard-coded-sleep-run-circuits-a1588164e61d5336.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 51d1139)

Co-authored-by: Renier Morales <renier@users.noreply.github.com>
ElePT pushed a commit to ElePT/qiskit-algorithms that referenced this pull request Jul 27, 2023
* Fix hard-coded sleep in run_circuits

Fixes Qiskit/qiskit#7792

Since all the places where the `_safe_get_job_status` happen in
a context where the intended wait/sleep time is available, we can
add the argument with the value to remove the hard-coded 5 seconds.

* run black

* add reno fix note

* grammar

* revising note accuracy

* Update releasenotes/notes/fix-hard-coded-sleep-run-circuits-a1588164e61d5336.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run_circuits is using a hardcoded sleep when checking job status
5 participants