-
Notifications
You must be signed in to change notification settings - Fork 185
Add new channel and allow instance selection with new platform #2239
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
Conversation
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.
Thanks @kt474, I took an initial look at the PR and I see that some details need to be ironed out and in general is a tricky feature to develop. I left a few comments here and there (some really minor), but I think that my main points are:
-
The documentation of the "ibm_cloud" channel should stay while the option is supported, even if it's not the recommended one. We should extend the documentation with the new "ibm_quantum_platform" channel, make sure the new name is used in the examples and deprecation message, and mention that it's the new default and follows the same configuration path as "ibm_cloud". After the alternative is in place, we can deprecate "ibm_cloud" explicitly. The new name is too close to "ibm_quantum" and many users will naively assume that it is replacing it making the migration even more complex.
-
Regarding your 2 questions from the PR description:
- Should service.backends() return all backends available through all instances?
- Fetching all instances then fetching backends for every instance is very slow (ntc 5779 would help)
My take is that with any change that we make that adds API calls, we should make sure users are not hit with a performance regression if they aren't getting anything out of it (so if you really need this functionality, sure, you might be willing to sacrifice speed, but some users won't need it). So I would only really fetch all instances if no instance
or default_instance
is set, and would only iterate over all instances and backends in that case. Also brainstorming here, this might be a good opportunity to try async API calls, which might mitigate the performance hit a bit?
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.
Sharing some conclusions from our internal discussion on how to advance with this PR:
- We will not add
default_instance
, and useinstance
instead. We will document the parameter insave_account
as: “This is an optional parameter. If set, it will define a default instance for service instantiation, if not set, the service will fetch all instances related to a specific token” (or something along these lines). We will add this change in behavior to the reno, mentioning that it’s now optional and that not setting it gives access to all instances. We will also let the docs team know about the change. - We will add the following new optional parameters to both
save_account
and__init__
:account
,region
,plans_preference
. Where if the value is set in init it will overwrite the default fromsave_account
. - If no instance is set during init, to do authentication, we can call IBM Cloud platform APIs to get all the service instances the user has access to
So I'm playing with this locally and some observations, right now if you do: QiskitRuntimeService.save_account(channel="ibm_quantum_platform", token="...") it saves an |
…nto cloud-migration
Allow for multiple clients in cloud service
…ified for non-default regions (non-US)
bd1d2dd
to
cb3910a
Compare
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 believe that the PR is finally in a mergeable state, thanks for the effort @kt474. The service seems to work as I expect it to, at least in terms of instance selection, prioritization and backend retrieval, retrieving former jobs and sessions, and erroring on wrong input combinations.
I would like to see a greater test coverage of these new changes, especially in integration tests, as well as better documentation. But realistically we cannot block the PR on these, and they can be improved gradually independently of the release schedule.
Summary
Documented changes in 2239.feat.rst
Details and comments
Fixes #2236
Fixes #2237
Fixes #2147