Update to new Q network url format #740
Conversation
Also clean up various issues
nonhermitian
requested review from
ajavadia,
atilag,
delapuente,
diego-plan9 and
ewinston
as code owners
|
Please, @nonhermitian remember to add the "Fix #issue" string to your PR to keep them linked. |
There was a problem hiding this comment.
Looking good overall, and personally glad to be able to avoid passing the hub, group and project parameters to the tests, specially! Most of the comments are aimed at avoiding repetition and further reduce complexity, moving towards using solely the new format whenever possible.
As a self-reminder, we need to update the travis variables right before the PR is merged for both QE and IBMQ, using the new format.
qiskit/backends/ibmq/ibmqprovider.py
Outdated
| "Groups/{group}/Projects/{project}" | ||
| url = url.format(hub=hub, group=group, project=project) | ||
| warnings.warn( | ||
| "Passing hub/group/project is depreciated in qsikit 0.6+" |
There was a problem hiding this comment.
Small nit - can you add a . (dot and space) or something similar, for proper displaying of the warning? This line and the next one are actually concatenated when printing, which would result in ...qiskit 0.6+Use the ... (also can you tweak qsikit?)
qiskit/backends/ibmq/ibmqprovider.py
Outdated
| @@ -19,19 +20,24 @@ def __init__(self, token, url='https://quantumexperience.ng.bluemix.net/api', | |||
| hub=None, group=None, project=None, proxies=None, verify=True): | |||
| super().__init__() | |||
|
|
|||
| if any([hub, group, project]): | |||
There was a problem hiding this comment.
Seeing that this block of code is used in several places - can you move it to a separate function, in a place where it is easy to import (for example, qiskit._utils.parse_ibmq_credentials(url, hub, group, project) - a better name might be needed)? This would avoid duplication, and we can take care of the checking, transforming and deprecation raising in a single place.
| def _authenticate(cls, token, url, | ||
| hub=None, group=None, project=None, proxies=None, | ||
| verify=True): | ||
| def _authenticate(cls, token, url, proxies=None, verify=True): |
There was a problem hiding this comment.
Do you think it would be a good idea doing a similar change to IBMQProvider.__init__, removing the arguments? Even if the class is technically public, if the converting from the old format to the new one is handled in the upper layers, it might make sense to do so and specially might be the best time to do it (adding a note in the docstring).
There was a problem hiding this comment.
There are places, for example in the tests, were the Provider is called directly. That is why I left it there, but with the warning, so I do not break something unintentionally.
test/python/common.py
Outdated
| 'hub': os.getenv('IBMQ_HUB'), | ||
| 'group': os.getenv('IBMQ_GROUP'), | ||
| 'project': os.getenv('IBMQ_PROJECT'), | ||
| 'QE_URL': _url |
There was a problem hiding this comment.
Can you actually remove the lines below (327-333) and just leave this as:
kwargs.update({
'QE_TOKEN': os.getenv('IBMQ_TOKEN'),
'QE_URL': os.getenv('IBMQ_URL')
})
We basically have full control over those variables as it is exclusively used for the "travis jobs for premium" use case, and it is just easier to update the environment variable directly (I'll do it right before merging the PR).
| @@ -338,16 +342,22 @@ def _(*args, **kwargs): | |||
| discovered_credentials = discover_credentials() | |||
| if account_name in discovered_credentials.keys(): | |||
| credentials = discovered_credentials[account_name] | |||
|
|
|||
There was a problem hiding this comment.
Instead of adding this block, can you actually call the function somewhere in wrapper/credentials/__init__.py:discover_credentials()? There are several places in that function where return happens, so moving things a bit might be needed to ensure that, whenever credentials are returned by that function, they always are in the new style. Tackling it at that level will simplify things and avoid us having to explicitly check for the old ones in some cases (for example, in this block).
There was a problem hiding this comment.
Ok, this has been addressed.
test/python/common.py
Outdated
| }) | ||
| if (credentials.get('hub') and credentials.get('group') and | ||
| credentials.get('project')): | ||
| args[0].using_ibmq_credentials = True |
There was a problem hiding this comment.
As a follow-up to the previous comment, we still need this assignment to be made if IBM Q credentials are being used. Something not extremely clean would work (if 'Hubs' in ...), although using a regular expression would be even better.
There was a problem hiding this comment.
Thanks @nonhermitian ! I added some small tweaks to the function, in order to clarify a bit its usage (ie. _ibmq_credentials_parser() makes is slightly more difficult to infer the purpose of the function than if using a verb in the function name) and for making the warning more similar to the existing ones.
Thanks as well for updating the documentation, which is something that is easy to miss! 👌 We should be good to go once the tests pass.
* Use new Q network url * lint fixes * Update readme for new url Also clean up various issues * remove hub/group/project from tests * remove temp notebook * Review updates * minor changes + changelog * update docs for new registration * minor text change to docs * Tweaks to _ibm_qcredentials_parser
Fix #736
Summary
The Q network now uses:
"https://q-console-api.mybluemix.net/api/Hubs/{hub}/Groups/{group}/Projects/{project}"as the URL, replacing the need to pass hub/group/project information to register and related functions.
Details and comments
Here we switch to the new format, and give depreciation warnings when the old hub/group/project style is passed.