-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
get jobs from IBM Q backend #501
Conversation
@ewinston I think this requires an update to IBMQuantumExperience 1.9.2 (should go in |
qiskit/_result.py
Outdated
self._result['result'] += other._result['result'] | ||
return self | ||
else: | ||
raise QISKitError('Result objects have different configs and cannot be combined.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we still need to do some checks in order to make sure the results can be combined, and if not raise this QISKitError? For example one of the checks is to make sure they come from the same backend. Previously this was done based on qobj
, but I think the result
has enough information for this check too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously it checked that the 'config' section of the qobj matched, however, perhaps we can just match the backend_name since I don't think 'shots' or 'max_credits' mismatch should raise an error.
qiskit/_result.py
Outdated
else: | ||
raise QISKitError('Result objects have different configs and cannot be combined.') | ||
self._result['result'] += other._result['result'] | ||
return self | ||
|
||
def __add__(self, other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the docstring of this method need updating? I think result
doesn't have a qobj_id
anymore.
test/python/test_reordering.py
Outdated
from qiskit.wrapper import register, available_backends, get_backend, execute | ||
from qiskit import QuantumJob | ||
from qiskit.wrapper import register, available_backends, get_backend | ||
from qiskit.wrapper import compile as qcompile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep this compile
instead of qcompile
for consistency with the rest of the code base? it won't collide with the compile keyword after this import. it has been used as compile()
, for example in wrapper.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this raised a style or linting flag but I'll retry.
qiskit/backends/ibmq/ibmqbackend.py
Outdated
Args: | ||
limit (int): number of jobs to retrieve | ||
skip (int): starting index of retrieval | ||
status (None or JobStatus): only get jobs with this status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to make this string for easier use? e.g. "running", "done", "queued", ..
if not, then I think this docstring should be updated with some examples, e.g. JobStatus.RUNNING, JobStatus.Done, JobStatus.QUEUED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this questions the use of enumerated types perhaps the docstring can just be updated for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks easy to also allow the string representation.
@@ -33,7 +33,6 @@ class Result(object): | |||
|
|||
Internal:: | |||
|
|||
qobj = { -- the quantum object that was complied --} | |||
result = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think now that Result._qobj
is removed, we can unpack Result._result
, and not have so many levels of indirection. That means having Result._job_id
, Result._status
, Result._result
, where the latter contains all the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds ok to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can leave this for a subsequent PR since it's not directly related to this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think we should leave this, also because it's not clear how qobj and result may be represented together later.
I think the most important thing in this PR is to not give errors when metadata
is unavailable from a past job.
qiskit/backends/ibmq/ibmqjob.py
Outdated
if 'metadata' in circuit_result: | ||
circ = circuit_result['metadata'].get('compiled_circuit') | ||
else: | ||
raise QISKitError('result object missing metadata for reordering bits') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This get raised when calling job.result()
for all jobs that were run prior to the new metadata
field, which seems quite recent. Should it at least return something, even though it could potentially miss some reorderings? Otherwise this PR is not immediately usable if one goes searching for past jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can return and log warning.
8caca6e
to
7ce53ea
Compare
Can we provide an easy method to get the date/time of execution of a job? I think that's useful for filtering, when someone gets a long list of past executions. It could go in the |
Other than that I think everything looks pretty great! |
qiskit/backends/ibmq/ibmqbackend.py
Outdated
status = JobStatus[status] | ||
while len(job_list) < limit or len(job_info_list) < limit: | ||
base_index += limit | ||
job_info_list = self._api.get_jobs(limit=limit, skip=base_index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to make repeated API calls here? Seems like the API should be able to return all the jobs at once. If it is not, perhaps we should update the API.
qiskit/backends/ibmq/ibmqbackend.py
Outdated
backend_name = self.configuration['name'] | ||
job_list = [] | ||
base_index = skip | ||
job_info_list = self._api.get_jobs(limit=limit, skip=base_index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit of code restructuring:
I think the 2 calls to the API here can be made just one. Place it in the first line of the while
loop. Then if all the jobs are retrieved in the first call, the while loop terminates. The way it is written right now, at least 2 calls are made every time.
@ewinston this looks good to merge to me. Can you please confirm that the TODO section of the original PR description is done now? |
@ajavadia yes I believe these are done now. |
The new speed improvement is really good. Mine went from 13 secs to 0.9 secs |
qiskit/backends/ibmq/ibmqjob.py
Outdated
job_instance._status_msg = None | ||
job_instance._cancelled = False | ||
job_instance._is_device = is_device | ||
job_instance._from_api = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this flag being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, thanks
# pylint: disable=arguments-differ | ||
while self._status == JobStatus.INITIALIZING: | ||
if self._future_submit.exception(): | ||
raise IBMQJobError('error submitting job: {}'.format( | ||
repr(self._future_submit.exception()))) | ||
time.sleep(0.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would encourage to at least change these loops for the ._wait_for_submmiting()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this seems like a performance improvement but it risks breaking things perhaps I can leave this for a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
qiskit/backends/ibmq/ibmqjob.py
Outdated
@@ -97,12 +158,12 @@ def cancel(self): | |||
@property | |||
def status(self): | |||
if self._status == JobStatus.INITIALIZING: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the _wait_for_submitting() there's no need for this check as well
qiskit/backends/ibmq/ibmqjob.py
Outdated
return self._job_id | ||
while not self._id: | ||
# job is initializing and hasn't gotten a id yet. | ||
time.sleep(0.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_wait_for_submitting()
:)
qiskit/backends/ibmq/ibmqjob.py
Outdated
while not (self.done or self.cancelled or self.exception): | ||
if timeout is not None and timer >= timeout: | ||
job_result = {'job_id': job_id, 'status': 'ERROR', | ||
# qobj = self._q_job.qobj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove commented code. There's no need for this having git :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
qiskit/backends/ibmq/ibmqjob.py
Outdated
logger.info('status = %s (%d seconds)', api_result['status'], timer) | ||
api_result = self._api.get_job(job_id) | ||
logger.info('status = %s (%d seconds)', api_result['status'], | ||
elapsed_time) | ||
|
||
if 'status' not in api_result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, seems like there is the possibility of a job having the status of CANCELLED
, so it has not results. We should be treating this status in the conditionals as well.
qiskit/backends/ibmq/ibmqjob.py
Outdated
# id) | ||
api_result = None | ||
start_time = time.time() | ||
while not (self.done or self.cancelled or self.exception or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sending at least one more request.
qiskit/backends/ibmq/ibmqjob.py
Outdated
'result': api_result['status']} | ||
return Result(job_result, qobj) | ||
return Result(job_result) | ||
|
||
if self.cancelled: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will send another request to the server, I would just check against api_result['status']. There's no way a job can be canceled by the user while is calling result
, as this is a blocking call.
qiskit/backends/ibmq/ibmqjob.py
Outdated
'status': api_result['status'], | ||
'used_credits': api_result.get('usedCredits'), | ||
'result': job_result_list} | ||
# logger.info('Got a result for qobj: %s from remote backend %s with job id: %s', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove commented code (or leave the trace there :))
self.assertTrue(isinstance(job.id, str)) | ||
self.log.info('time to get job statuses: %0.3f s', time.time() - start_time) | ||
|
||
def test_retrieve_job(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make a test for a Job with multiple circuits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_run_simulator does this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
This bug would cause the method to miss first "limit" number of jobs.
Previously retrieving jobs without metadata would raise an exception. Now a warning is issued. Also, the property "creationDate" was added to ibmq jobs.
Previously backend.jobs() more api calls than it needed. Also, ibmqjob.job_id is now ibmqjob.id
This commit incorporates improvements from PR Qiskit#515 by @delapuente to handling job status such that it makes fewer api calls. Co-authored-by: [email protected]
for status of RUNNING or QUEUED or DONE the status_msg would indicate "initializing"
when calling job.status for a server with a suspended queue the command would hang.
qiskit/backends/ibmq/ibmqbackend.py
Outdated
if isinstance(status, str): | ||
status = JobStatus[status] | ||
while len(job_list) < limit or len(job_info_list) < limit: | ||
job_info_list = self._api.get_jobs(limit=limit, skip=base_index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest version of IBMQuantumExperience package (1.9.4), we can pass a filter
to the get_jobs(), so we can filter by backend name and job status, making this whole algorithm way simpler and reducing the potential huge number of requests to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great! Thanks for getting that through.
database filtering was introduced in API 1.9.4, which has much better performance, however it doesn't seem to filter on status properly for real devices.
After talking with @ewinston we are going to leave |
* add jobs() method to retrieve all jobs from a backend * job cancellation improvements for hubs * add retrieve_job() method for ibmq backends * add circuit names to non-qobj ibmq job submission * remove qobj from Result * add temporary hack for bit reordering * allow retrieving old jobs without metadata * property "creationDate" was added to ibmq jobs.
This PR allows one to get IBMQJob objects for previously submitted jobs to an IBM Q backend.
job_list = backend.jobs()
job = backend.retrieve_job(job_id)
Description
TODO:
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: