Adding test suite for testing IBMQJob in isolation.#515
Conversation
delapuente
requested review from
ajavadia,
atilag,
diego-plan9 and
ewinston
as code owners
delapuente
force-pushed
the
test-ibmqjob-in-isolation
branch
from
0becd31 to
a105d5b
Compare
There was a problem hiding this comment.
I think the expectation for timeout will be the maximum time for the result call to return. Perhaps we can instead start a clock here (timeout_time = time.time() + timeout) and pass timeout_time to _wait_for_initialization and _wait_for_job.
There was a problem hiding this comment.
How does this PR handle exceptions raised by self._future_submit which is done async and may occur later?
There was a problem hiding this comment.
Notice that reading status will be stuck in the INITIALIZING state until the initialization routine _submit changes the internal job status.
All the errors happening inside the _submit() are silenced (except for one case I just realize, thanks!) and stored inside the _exception member, the internal status progress to JobStatus.ERROR in these cases.
This causes all future reads to status to skip querying the API since the job is in one of the final states.
Both tests test_status_flow_for_invalid_job and test_status_flow_for_throwing_api test the job in the cases when the job is invalid (and the API rejects it by raising an ApiError) or the network side fails somehow (and the API returns an errored result) respectively. Anyway, I'll reinforce the tests in the next commits, checking the exceptions to get more confidence in the paths we are following.
Hope it helps.
There was a problem hiding this comment.
There is an error in my assumptions regarding _submit() progressing the job to a final state. This final assignment happening at the end of the method invalidates de second paragraph of the explanation. I'll fix in the next commit. Thanks for the heads-up.
delapuente
force-pushed
the
test-ibmqjob-in-isolation
branch
2 times, most recently
from
ffd3ba0 to
acdfd57
Compare
|
Everything is fixed now. What about a second review? |
delapuente
force-pushed
the
test-ibmqjob-in-isolation
branch
4 times, most recently
from
a3fd9e7 to
e13056d
Compare
There was a problem hiding this comment.
For the new files, can you adjust the license header so it follows the format recently introduced by #550 ?
# -*- coding: utf-8 -*-
# Copyright 2018, IBM.
#
# This source code is licensed under the Apache License, Version 2.0 found in
# the LICENSE.txt file in the root directory of this source tree.
# pylint: disable=something-only-if-there-is-no-other-way
"""
Description of the module ...
"""
from qiskit import ...
e13056d to
bf0efec
Compare
This commit incorporates improvements from PR Qiskit#515 by @delapuente to handling job status such that it makes fewer api calls. Co-authored-by: salva@unoyunodiez.com
delapuente
force-pushed
the
test-ibmqjob-in-isolation
branch
from
bf0efec to
9eaf078
Compare
|
@delapuente @ewinston not sure how to handle this PR. As this is going to test Jobs, and Ericks PR #501 (jobs from Backend) is hughly changing IBMQJob... we would need to have them sync. |
|
Since #501 is tagged as "high priority" and it's an earlier PR perhaps it should be the other way around. |
delapuente
force-pushed
the
test-ibmqjob-in-isolation
branch
from
f5daa11 to
06c3a10
Compare
|
@delapuente is this PR rebased after #501 got merged? |
|
Does this solve #234? |
Yes. Since the implementation of IBMQJob changed noticeably, I've discarded most of my changes from the original PR but that's the only difference.
Not yet. It contributes to that but it is not replacing the original IBMQJob suite and more work is needed in other parts of the library. |
There was a problem hiding this comment.
Good point, but the Result has an ERROR status, so for the user something wrong happened with this Job. Should we state in any way, that this may not be a real Job error?... like having another status for "TIMEOUT" or something similar.
There was a problem hiding this comment.
Let's mark this for future improvements... we need to merge this asap.
There was a problem hiding this comment.
See #588 for the follow-up.
There was a problem hiding this comment.
When job cancellation works on all IBM Q systems timeout could use that to cancel the job, which could be considered an error.
The test suite does not require network connection. It fakes the QJOB object and the API behaviour.
delapuente
force-pushed
the
test-ibmqjob-in-isolation
branch
from
06c3a10 to
290e28d
Compare
diego-plan9
left a comment
diego-plan9
left a comment
There was a problem hiding this comment.
Thanks everyone for the effort!
* Adding test suite for testing IBMQJob in isolation. The test suite does not require network connection. It fakes the QJOB object and the API behaviour. * Removing QuantumJob * Adding expected failure when cancelling during initialization
5 participants
The test suite does not require a network connection. It fakes the QJOB object and the API behaviour.
This commit partially refactors the
IBMQJob#status()method to avoid raising exceptions while reading a property.Motivation and Context
Current tests behave as integration tests so, in case of failure, it is not guaranteed the problem arises from IBMJob-related abstractions malfunctioning.
Checklist: