WIP Add new channel and allow instance selection with new platform #2239
Conversation
There was a problem hiding this comment.
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?
| # def list_instances(self) -> None: | ||
| # """""" | ||
| # pass | ||
|
|
There was a problem hiding this comment.
The method is defined later:
| # def list_instances(self) -> None: | |
| # """""" | |
| # pass |
| @@ -371,7 +371,7 @@ def usage_estimation(self) -> Dict[str, Any]: | |||
| @property | |||
| def instance(self) -> Optional[str]: | |||
| """For ibm_quantum channel jobs, return the instance where the job was run. | |||
| For ibm_cloud, `None` is returned. | |||
| For ibm_quantum_platform, `None` is returned. | |||
There was a problem hiding this comment.
The behavior for ibm_cloud should still be documented until the option is removed.
| instance: This is only supported for ``ibm_quantum`` runtime and is in the | ||
| hub/group/project format. | ||
| instance: In hub/group/project format if on the ``ibm_quantum`` channel. | ||
| IBM Cloud account crn if on the ``ibm_quantum_platform`` channel. |
There was a problem hiding this comment.
Same comment on still documenting "ibm_cloud" if it is still a valid channel, which it is until deprecation (then during the deprecation PR we can mark it as deprecated in the docstrings, but not remove the info until the feature is removed).
| if instance_with_backend: | ||
| # TODO how do we handle this logic here? | ||
| # fetching a backend not in the current account instance requires a | ||
| # different api client - this api client is tied to everything | ||
| self._client_params.instance = instance_with_backend | ||
| self._api_client = RuntimeClient(self._client_params) |
There was a problem hiding this comment.
This code seems to be overwriting the api client. How would it behave if a user let's say instantiates the service, asks for backend 1 in instance A, asks for a backend 2 in instance B and decides to start a session with backend 1? Would this work?
There was a problem hiding this comment.
That's a good point, I'l have to test that edge case - I don't think that would work. I can't think of any other way to handle this since the backend is ultimately always tied to its instance.
There was a problem hiding this comment.
Sharing some conclusions from our internal discussion on how to advance with this PR:
- We will not add
default_instance, and useinstanceinstead. We will document the parameter insave_accountas: “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_accountand__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
| @@ -1183,7 +1330,7 @@ def instances(self) -> List[str]: | |||
| """ | |||
| if self._channel == "ibm_quantum": | |||
| return list(self._hgps.keys()) | |||
| return [] | |||
| return [t["crn"] for t in self._all_instances] | |||
There was a problem hiding this comment.
This line returns an empty list for the case where no instance is given, but I would expect it to list all available instances, as when calling list_instances()
| self._client_params.instance = instance | ||
| self._api_client = RuntimeClient(self._client_params) | ||
| return self._api_client.list_backends() |
There was a problem hiding this comment.
You may have come up with a better solution to avoid overwriting the client, but in case it helps: I think that you could refactor the client_params object and/or the RuntimeClient object (I would have to look into the specifics) to be able to store more than 1 instance in the cloud case. This would allow to keep handles to all instances a user might need instead of imposing one instance at a time. They could be stored together with the backend name, so the client knows which instance to use to access a specific backend.
|
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 |
Fixed the issue where |
Summary
Documented changes in 2239.feat.rst
TODO:
api_client. This makes the logic a bit hacky/tricky - is there a way around this? Even just fetching thebackend configuration requires an instance.
Shouldservice.backends()return all backends available through all instances?Details and comments
Fixes #2236
Mostly fixes #2237