-
Notifications
You must be signed in to change notification settings - Fork 41
Improve integration with QiskitIBMRuntime #1707
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
base: main
Are you sure you want to change the base?
Conversation
41cc9fd
to
e615587
Compare
…lient methods, new integration tests.
9ca1252
to
a0cce46
Compare
Code has changed substantially since review.
…onfig, clean up code
Add release note
5756aca
to
869a2b5
Compare
return Response(json.dumps(ids)) | ||
runtimejobs = job.runtime_jobs.all() | ||
serializer = RuntimeJobSerializer(runtimejobs, many=True) | ||
return Response({"runtime_jobs": serializer.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.
I went multiple times back and forth with these endpoints. In the end decided to keep both the add and the list one for various reasons. I ran into limitations in result serialization, where the automatic pipeline would not return a dict for "runtime_jobs", instead it would return a list. I tried to figure this out but I think I need to improve my django knowledge. In any case:
- Making formatting changes is easier if we have a dedicated endpoint
- Keeping the add/list also adds transparency of what the endpoint is for in my opinion
- It creates separation of concerns. I don't see the need to have everything together in
retrieve
@@ -0,0 +1,82 @@ | |||
--- | |||
features: |
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 tried adding a release note, and realized that I am only used to communicating interface changes. I think we should also have API release notes but I am not sure how these should be structured.
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.
Yes, let's discuss the better way to handle 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.
I couldn't have time to look at this in-depth sorry, Elena. Just a little comment from the warnings that I saw in the tests.
Also, I saw that you didn't apply the refactor for the end-points to the new architecture. Do you prefer to do that in another PR? (it's fine for me, this one has changes enough so no problem).
upgrade: | ||
- | | ||
Enhanced ``job.stop()`` behavior to use the newly introduced features. ``job.stop()`` now attempts |
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.
If I remember correctly the interface job.stop
was deprecated in favor of job.cancel
to follow a similar UI as Runtime 👀
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.
Hmmm I did miss that, I am surprised I never saw the warnings running my tests...
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.
Aha, so only job.stop
was deprecated, not service.stop(job_id)
. Should we add a new service.cancel()
for consistency? Keeping track of these naming changes can be challenging.
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.
Anyways, addressed in afe300b.
@@ -0,0 +1,82 @@ | |||
--- | |||
features: |
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.
Yes, let's discuss the better way to handle this!
) | ||
|
||
# Initialize serverless folder for current user | ||
function = QiskitFunction( |
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 the error from the integration test comes from this change. Why are we removing this hello-world
? (a naive question, I don't remember in detail this configuration for the test, maybe @korgan00 has more context).
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, that makes sense, I originally had 2 of these conftests but then decided to move the new tests to the docker folder and use the "smallest" conftest. Up until this point I hadn't understood what this part of the code was for. I will undo this change.
Summary
This PR proposes a series of changes to allow a better integration with the
QiskitRuntimeService
.get_runtime_service()
. This helper function instantiates a runtime service pulling by default the authentication function from the environment variables. This simplifies the instantiation process from something like:to doing:
get_runtime_service()
doesn't only simplify the instantiation service, it also allows the API to store the runtime job ids and session ids created within a serverless job.get_runtime_service()
(this will NOT work if the function developers useQiskitRuntimeService
independently), users can retrieve the runtime job and session ids following the code below:The outputs of these methods are list of id strings that can be directly plugged into a
QiskitRuntimeService
with access to these jobs for all the usual operations. They can also be used to search in the job dashboards from IQP.Details and comments
This PR also updates the
job.stop()
method to attempt to authenticate aQiskitRuntimeService
using the credentials provided to theServerlessClient
and stop the runtime jobs associated to the serverless job id. This mechanism will only work if the credentials used to authenticate to aQiskitRuntimeService
in the function code MATCH those used to authenticate to theServerlessClient
, which is the general use-case when accessing a deployed function and the function code runsget_runtime_service()
(or an independently instantiatedQiskitRuntimeService
using environment variables).There are some cases where this mechanism won't work automatically. To cover these cases,
job.stop()
accepts aservice
input parameter where you can customize the runtime service used to stop the jobs:ServerlessClient
is instantiated with a dummy token/instance, these dummy credentials can't be used to instantiate aQiskitRuntimeService
. In these cases, you can still use thejob.stop()
functionality providing an independently instantiated service. For example:QiskitRuntimeService
(for example, to access devices in staging), even if the token an instance match, the automatic instantiation injob.stop()
won't be able to get the right service instance, as the runtime URL is not a parameter of theServerlessClient
. In these cases, you should also provide an independently instantiated service: