Improve user authentication handling #547

nonhermitian · 2018-06-07T16:55:18Z

Description

Work to make changes inline with #540.

Add ability to create, read, and write to local qiskitrc file.
Auto-register providers after registering once (if so desired).
Register via env variables.
Register automatically via Qconfig.py if in local dir.
Update documentation.

Summary of Changes Made

User credentials can now be stored in the qiskitrc file in the users HOME/.qiskit folder.
Calling register() with no arguments loads credentials from a Qconfig.py file in the cwd, the env vars QE_TOKEN, QE_URL, QE_HUB, QE_GROUP, and QE_PROJECT, then finally qiskitrc, in that order.
Provider base classes can now be named, and providers with the same name are not registered twice.
Can modify the qiskitrc file using store, get, and remove _credentials functions.
IBMQ provider is automatically named ibmq, where as the Q network provider is called qnet.
Can register both IBMQ and Q Network providers at same time.
Can remove a provider with new unregister() function.
Can register single provider (if many exist) by doing register(provider_name='MY_PROVIDER').
Added requires_ci test decorator for testing registration routines that would break a local users registration.

Motivation and Context

Make the end users life easier for registering providers.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

ewinston · 2018-06-07T18:06:17Z

qiskit/wrapper/configrc/_settings.py

+# limitations under the License.
+# =============================================================================
+
+QISKIT_RC_FILE = None


Is this file only for internal use or is it meant to be edited by a user?

Internal only.

It holds the location of the qiskitrc file.

nonhermitian · 2018-06-07T19:07:35Z

Currently there is a function in the wrapper that will register any provider information found in the qiskitrc file, env variables, or a Qconfig.py file in the cwd:

load_local_credentials()

To get credentials into the qiskitrc file you must first register the information manually at set the new kwargs: store_credentials=True and auto_register=True. The first stores the info in qiskitrc while the second indicates that the user wants the registration to be done automatically at import. If auto_register=False, then the registration is done only when load_local_credentials is called.

diego-plan9 · 2018-06-08T13:05:10Z

For the new files, can you adjust the license header so it follows the format recently introduced by #550 ?

# -*- coding: utf-8 -*-

# Copyright 2018, IBM.
#
# This source code is licensed under the Apache License, Version 2.0 found in
# the LICENSE.txt file in the root directory of this source tree.

# pylint: disable=something-only-if-there-is-no-other-way

"""
Description of the module ...
"""
from qiskit import ...

nonhermitian · 2018-06-08T17:30:58Z

Dropped 'make generating Qconfig.py easier' from the scope as our goal is to move away from them.

nonhermitian · 2018-06-10T02:24:20Z

@diego-plan9 , the online tests are not running for this PR. In order to test the API registration functions, I need to run the tests in CI only, otherwise I would kill the credentials on the users local machine.

ajavadia · 2018-06-12T15:54:25Z

qiskit/wrapper/_wrapper.py

+    """
+    _registered_names = [p.name for p in _DEFAULT_PROVIDER.providers]
+    if provider_name is None:
+        return _registered_names


why return all the _registered_names if nothing is passed as provider_name?
shouldn't you make the argument mandatory, since otherwise it doesn't make sense to call unregister?

Yeah, I didn't want to make another function available_providers just to get the list to pass to unregister, so I did this. Perhaps I should just make the new function. In principle, it is possible for credentials to come from multiple locations, and they would not show up as stored credentials.

diego-plan9

It turned out to be a lengthy review - but the summary is that functionality-wise is looking great and covers the goals very nicely (in particular, thanks a lot for the effort in the documentation), and that with a bit more focus on the modularity and architecture aspects it will be even nicer! As a matter of fact, nearly all the comments are geared towards simplifying the implementation and "moving things around a bit" in the hopes of achieving a cleaner implementation that breaks it into finer pieces and fits better into the core - looking forward to the next round!

There is also the issue of the tests - I did not review them yet following the mention that they were a bit WIP at the moment, but I'm confident we can come up with a solution that allows them to run locally eventually.

diego-plan9 · 2018-06-12T17:04:44Z

qiskit/wrapper/configrc/_configrc.py

+import ast
+import configparser
+from qiskit import QISKitError
+from qiskit.wrapper.configrc import _settings as rc_set


Nit: can we please avoid import xxx as yyy in general, for readability?

QISKit already uses the following:

import numpy as np import scipy.linalg as la import scipy.sparse as sp import scipy.sparse.csgraph as cs import networkx as nx from sympy import Number as N from qiskit.qasm import _node as node import ply.lex as lex from . import _node as node import ply.yacc as yacc import matplotlib.pyplot as plt

why is my usage of the same functionality problematic?

Again, for readability - in general they do not provide tangible benefits, but have the drawback add another level of indirection in the code, which hinders parsing the code at a glance. The examples you list are either references to other libraries (where some of them, as the numpy alias, are relatively widespread) with only three of them related to qiskit modules (and part of code that has not changed much since the beginning of the project).

diego-plan9 · 2018-06-12T18:43:10Z

qiskit/wrapper/configrc/_configrc.py

+# the LICENSE.txt file in the root directory of this source tree.
+
+"""
+A module for generating and manipulating the qiskitrc file.


Can you add to the documentation the format of the .qiskitrc file (somewhere in the sphinx doc - I'm unsure about the best place, but as long as it is there we can move it around)?

And on that topic, can we rethink the format itself? It seems that currently it is allowing dicts and evaluating the contents back when needed, which might be not too well suited to our use case. What about simplifying it to just one value per key, as in:

[ibmq] token=123456 url=http://foo [somethingelse] token=5678 url=http://bar group=x ...

Since we don't really have much more use for the sections in the .qiskitrc currently, it would make it more straightforward when editing manually, and at the same time probably allow a cleaner and safer implementation.

I originally did that but it is not very clean, and the minute we add other stuff to the file we run into trouble. Since we can store a dict, that seemed to be the cleanest way.

It is preferable to have a default .qiskitrc file with a template discussing the options and syntax rather than forcing the user to take a look at the code.

Again, I do not want them to see the file. I just want them to use the provided functions to register.

diego-plan9 · 2018-06-12T18:52:59Z

qiskit/wrapper/configrc/_configrc.py

+
+"""
+A module for generating and manipulating the qiskitrc file.
+"""


Regardless of the configuration format, can we refactor this module to make it more pythonic, and simplify its usage - ideally be able to something like:

> credentials = read_credentials_from_file() { 'acount1': {'token': 12345, 'url': 'http://fo'}, ... } > credentials['account1']['token'] = 'mynewtoken' > write_credentials_to_file(credentials)

or even go a bit further and have a proper credentials class. This way we can centralize the access to the qiskitrc file, and removing, updating, and checking credentials becomes simply modifiying the dict/class - for the intermediate functions (if needed) between reading and writing, also passing ConfigParser around would probably be a better approach than having to open and close the config file in several places.

This would also help for the environment variables case: if the function that checks for the environment variables returns the same structure, switching between them becomes simpler.

Please note as well that the names (and the structure) used in the review as a whole are tentative - better names than the ones sketched on the comments are needed ™️

You can already do what you are asking:

creds = get_credentials('ibmq') creds['token'] = 'MY_NEW_TOKEN' store_credentials(**creds, overwrite=True)

It seems it cannot be done at the granularity level that was suggested in the comment, which was the important bit of it.

diego-plan9 · 2018-06-12T18:55:14Z

qiskit/__init__.py


 # Import the wrapper, to make it available when doing "import qiskit".
 from . import wrapper

+from .wrapper.configrc._configrc import (has_qiskit_configrc, get_credentials,
+                                         store_credentials, remove_credentials)
+from .wrapper._wrapper import qiskitrc_register_providers


Could you re-check if all these functions are really meant to be invoked directly by the end user? We should be careful about not adding too many things to the root qiskit namespace unless really needed.

A good point, let me see what we really need to expose for average user.

diego-plan9 · 2018-06-12T20:07:28Z

qiskit/wrapper/_wrapper.py

@@ -21,35 +27,159 @@
 _DEFAULT_PROVIDER = DefaultQISKitProvider()


-def register(token, url='https://quantumexperience.ng.bluemix.net/api',
+def register(token=None, url='https://quantumexperience.ng.bluemix.net/api',


Can we refactor a bit this function as well, aiming for simplifying the original use case of the function (ie. the current functionality) and treat the case of a missing token in a more modularized form? Similar to:

def register(token=None, ...): if not token: token, url, hub, ... = _load_token(...) provider = provider(token, ...) def _load_token(....): # try _load_token_from_qconfig(), _load_token_from_envs(), _load_token_from_qiskitrc()

The idea is again to try to better encapsulate the functionality and the complexity (perhaps even moving the token-finding-in-all-places functionality as a whole to the wrapper.configrc module, renaming in a more generic form).

Yes, it can be cleaned up. I started adding functionality a bit hastily at the end.

diego-plan9 · 2018-06-12T20:12:04Z

qiskit/wrapper/configrc/_settings.py

+
+QISKIT_RC_FILE = None
+
+REGISTER_CALLED = 0


Would it be possible to make better use of qiskit-core/qiskit/wrapper/defaultqiskitprovider.py for keeping track of the registered backends and their names, instead of a global variable? Currently it uses a list of providers, but by extending it to a dict it seems it would simplify the handling of the providers names and split the functionality better - with the big benefit of having a single instance where that information is stored instead of global scope.

The _settings does not keep track of the registered backends, it only is used for storing runtime variables. The backends are stored as usual, using the new Provider.name functionality to understand which are which.

diego-plan9 · 2018-06-12T20:13:40Z

qiskit/wrapper/configrc/_settings.py

+"""Module that holds the runtime location of qiskitrc file.
+"""
+
+QISKIT_RC_FILE = None


Can this variable contain a value other than ~/.qiskit/qiskitrc? In the usual spirit, if we can avoid having a global variable, other implementation would be preferred.

It can be None. It is used not only to point to the qiskitrc file, but to tell the routines if it can not be found. It is set from None to the location of the qiskitrc file at runtime if it is there.

diego-plan9 · 2018-06-12T20:19:18Z

qiskit/backends/baseprovider.py

@@ -17,7 +17,7 @@ class BaseProvider(ABC):
    Base class for a backend provider.
    """
    def __init__(self, *args, **kwargs):
-        pass
+        self.name = 'base'


Can we re-think the need for having a provider name as a required instance attribute? It seems the main reason for introducing the parameter is for helping with the changes of the implementation of register(), and by design the Base classes should ideally be as minimal as possible to avoid side effects and constraints - please see the note in IBMQProvider.

You could, but then it would make other code more complicated as one would have to check for a provider name, or whether it even exists or not, and what to do in that case.

It is preferable to keep the complexity outside the core to not increase coupling.

What do you suggest doing if there are multiple IBMQ providers to register/unregister? I think people are fairly comfortable with the idea of a provider name.

The solution depends on the specific use-case. For instance:

If we don't allow several instances of the same provider, simply talking about them is wrong. There is no identity problem and you can always query type(provider).

If we do allow several instances of the same provider, then they can be stored in a dictionary as @diego-plan9 suggested you: { "name": provider_instance }.

...

So, would you mind to clarify why do you need names so we can come up with the proper solution both meeting your needs and without introducing complexity?

The whole point of the name is that we may have multiple IBMQProvider instances. It also simplifies iterating through the list of registered providers and simplifies the unregistration process.

In that case, use @diego-plan9's name dictionary strategy instead. The DefaultQISKitProvider which is defined as a "meta provider" is intended to add extra functionality like the one you're suggesting. This solution would meet your API usage expectation without introducing extra complexity to the core.

diego-plan9 · 2018-06-12T20:28:22Z

qiskit/backends/ibmq/ibmqprovider.py

+            elif 'q-console' in url:
+                provider_name = 'qnet'
+            else:
+                raise QISKitError('Unkown IBMQ provider.')


Following up on the comment in BaseProvider, I think this introduces some tiny yet undesirable side effects: in particular, makes provider_name to be effectively required when creating an IBMQProvider instance if using non-standard URLs without real benefits when using the provider directly - ie. the .name is only relevant for register() and their related functions, which compromises a bit the decoupling. It also seems that in this revision, it is possible for two instances to have the same name, which I'm not sure if it is by design and would require some clarification (ie. what is the relation between a provider and its name - always the same name, as happens with the LocalProvider, a default name but potentially dependent on the user needs, as in this one, etc?).

Instead of that - what about delegating the handling of provider names to DefaultQISKitProvider? It is arguably the purpose of that class, and it seems that by using a "dict of provider_name: Provider" there would be enough to cover the needs introduced by this PR and at the same time avoid introducing application-dependent changes in the base classes.

The name is indeed used only to register and unregister at the moment. However, as discussed in our meeting, there could be cases where there are multiple IBMQProvider instances registered, and I need a way to address them, e.g. to unregister/register one and not the other. I demoed one such example of this.

If you can think of a better way to accomplish the same task, then I am curious to hear your suggestion.

diego-plan9 · 2018-06-12T20:36:14Z

qiskit/wrapper/_wrapper.py

+        QISKitError: if the provider name is not recognized.
+    """
+    if provider_name == 'core':
+        raise QISKitError("Cannot unregister 'core' provider.")


Very random thought: why do we want to restrict unregistering the core provider?

Because we can not register it, and what would be the use case to remove the 'core' solvers?

delapuente

@nonhermitian the ideas in this PR are good and seems to simplify the registration process. Nevertheless, I don't see the point in adding name to the local and IBM Q providers. What is the use-case for that new functionality?

delapuente · 2018-06-18T14:14:46Z

README.md

-   `config` variable with the values you can find on your IBM Q account
-   page.
+4. If you have access to the IBM Q Network features, you also need to pass the
+   values for your url, hub, group, and project found on your IBM Q account


It would be convenient to link IBM Q account with the actual URL and a complete example of how to pass these extra parameters.

This is the same text that was there before, and is only of interest to hub members.

Non-blocking although It is never late for clarifying the docs.

delapuente · 2018-06-18T14:18:16Z

doc/install.rst

+
+For more advanced users, it is possible to load API credentials from 
+environmental variables.  Specifically, one may set `QE_TOKEN`,
+`QE_URL`, `QE_HUB`, `QE_GROUP`, and `QE_PROJECT`.  These can then be registed 


Replace registed with registered.

delapuente · 2018-06-18T14:24:13Z

doc/install.rst

+
+    from qiskit import register
+
+    register()


Documenting the new way of registering by providing three sections with exactly the same example can be misleading. The user can think there is an error in the example or something magic is happening behind the scenes. My recommendation is to introduce register() first saying it is an automatic method which will look for configuration in several origins according to an order, then introduce the origins in separate sections.

Good point.

delapuente · 2018-06-18T14:35:39Z

qiskit/wrapper/_register.py

+        project (str): The project used for online backend.
+        proxies (dict): Proxy configuration for the API, as a dict with
+            'urls' and credential keys.
+        verify (bool): If False, ignores SSL certificates errors.


This can cause security issues, let's name it more explicitly such as ssl_verification.

That exact same text comes from the old register function.

nonhermitian · 2018-06-18T17:27:16Z

qiskit/wrapper/_wrapper.py

+        project (str): The project used for online backend.
+        proxies (dict): Proxy configuration for the API, as a dict with
+            'urls' and credential keys.
+        verify (bool): If False, ignores SSL certificates errors.


Again, this is just the old text.

delapuente · 2018-06-18T16:24:04Z

qiskit/_quantumprogram.py

@@ -630,7 +630,7 @@ def get_initial_circuit(self):
    ###############################################################

    def set_api(self, token, url, hub=None, group=None, project=None,
-                proxies=None, verify=True):
+                proxies=None, verify=True, provider_name='ibmq'):


QuantumProgram instances should be agnostic of providers and so, we should not provide a default value for the provider_name parameter.

I believe I forgot to remove this in the latest version. Will remove.

delapuente · 2018-06-18T16:37:53Z

test/python/test_quantumprogram.py

-        )
-        # SDK will throw ConnectionError on every call that implies a connection
-        self.assertRaises(ConnectionError, qp.set_api, FAKE_TOKEN, FAKE_URL)
+    # def test_offline(self):


Remove dead code or disable the test explicitly with unittest.skip

delapuente · 2018-06-18T16:48:32Z

qiskit/wrapper/configrc/_configrc.py

+# the LICENSE.txt file in the root directory of this source tree.
+
+"""
+A module for generating and manipulating the qiskitrc file.


It is preferable to have a default .qiskitrc file with a template discussing the options and syntax rather than forcing the user to take a look at the code.

delapuente · 2018-06-18T16:57:07Z

qiskit/wrapper/configrc/_settings.py

+"""Module that holds the runtime location of qiskitrc file.
+"""
+
+QISKIT_RC_FILE = None


Actually, this is not a setting but shared state for the functions in _configrc.py. I'm not a friend of using OOP when it is not necessary, so a module works form me. This variable should be in _configrc.py as a module-level.

I do not want the user to see the qiskitrc file at all. I want them to simply pass a token and possibly url to the store_credentials function, or register.

I think this reply is misplaced but it is fair.

delapuente · 2018-06-18T16:59:22Z

qiskit/wrapper/configrc/_settings.py

+
+QISKIT_RC_FILE = None
+
+REGISTER_CALLED = 0


Same here, this is related to _register.py only and so, it should live there.

QISKIT_RC_FILE is used in several places. The other one can be moved.

Actually REGISTER_CALLED is also used in three different modules.

According to my search results of your branch, the name REGISTER_CALLED appears indeed in two different places (in addition to this file): _register.py and _wrapper.py whereas QISKIT_RC_FILE appears only in _settings.py. So, you can move QISKIT_RC_FILE there.

The problem with REGISTER_CALLED arises from the need for communicating between _wrapper.py and _register.py. This is one solution:

Move the registration code in _wrapper.py into another function _register_using_origin_chain. And make register function to delegate into this new function.

Move this function inside _register.py and adjust the imports in _wrapper.py.

Now all REGISTER_CALLED only appears in _register.py and you can turn the global variable into a module-level one inside _register.py.

I can do this. It makes the old register function just a wrapper for one inside of _register but I am fine with that.

nonhermitian · 2018-06-18T17:27:16Z

qiskit/wrapper/_wrapper.py

+        project (str): The project used for online backend.
+        proxies (dict): Proxy configuration for the API, as a dict with
+            'urls' and credential keys.
+        verify (bool): If False, ignores SSL certificates errors.


Again, this is just the old text.

delapuente

Hi Paul,

Please, address the requested changes and clarify a couple of matters:

The need of name so we can come up with an alternative not increasing the complexity of core classes.
The use-case of unregister.

delapuente · 2018-06-19T07:06:38Z

README.md

-   `config` variable with the values you can find on your IBM Q account
-   page.
+4. If you have access to the IBM Q Network features, you also need to pass the
+   values for your url, hub, group, and project found on your IBM Q account


Non-blocking although It is never late for clarifying the docs.

delapuente · 2018-06-19T07:08:47Z

doc/install.rst

+where `MY_API_TOKEN` should be replaced with your token.
+
+If you are on the IBM Q network, you must also pass `url`,
+`hub`, `group`, and `project` arguments to `store_credentials`.


What to do if multiple IBMQProviders are created. 2) How best to search through multiple registered providers [LocalProvider, IBMQProvider1, IBMQProvider2, ExternalProvider1,...] so that double registration can not happen, and we know which providers are loaded so that one or more may be unregistered. Unregister was requested elsewhere.

delapuente · 2018-06-19T08:24:54Z

qiskit/backends/baseprovider.py

@@ -17,7 +17,7 @@ class BaseProvider(ABC):
    Base class for a backend provider.
    """
    def __init__(self, *args, **kwargs):
-        pass
+        self.name = 'base'


The solution depends on the specific use-case. For instance:

If we don't allow several instances of the same provider, simply talking about them is wrong. There is no identity problem and you can always query type(provider).

If we do allow several instances of the same provider, then they can be stored in a dictionary as @diego-plan9 suggested you: { "name": provider_instance }.

...

So, would you mind to clarify why do you need names so we can come up with the proper solution both meeting your needs and without introducing complexity?

delapuente · 2018-06-19T08:26:01Z

qiskit/wrapper/configrc/_settings.py

+"""Module that holds the runtime location of qiskitrc file.
+"""
+
+QISKIT_RC_FILE = None


I think this reply is misplaced but it is fair.

nonhermitian · 2018-06-19T13:18:13Z

@delapuente please fork my branch and make the stylistic changes you see fit. Also, @diego-plan9, as we were both assigned to this registration update task, please fork my branch, and make any changes you think need to be done.

diego-plan9 · 2018-06-20T08:20:26Z

@delapuente please fork my branch and make the stylistic changes you see fit. Also, @diego-plan9, as we were both assigned to this registration update task, please fork my branch, and make any changes you think need to be done.

I'll go for it, as there are a number of issues still to be addressed. In the meantime, can you update on the current state of the tests - do you have a proposal or an implementation on how to get them back into shape?

ajavadia · 2018-06-21T01:31:28Z

Can we add one line to the CHANGELOG for this?

diego-plan9 · 2018-06-21T08:08:51Z

Labeling as on hold, as mentioned in #584 .

diego-plan9 · 2018-07-10T12:55:45Z

After discussion in private last week, I have proceeded retaking this PR and doing a bit of cleanup, as it had diverged quite a lot in regards to the recent changes (the register() changes in master were quite conflicting with the current status). For safety, I created a "backup" branch that has the same contents as this PR had just before this comment, and proceeded with squashing the ~70 at 20d12f0, which includes rebasing with current master.

In the hopes of being able to include the PR in 0.6, I went ahead and narrowed down the scope of this PR even further: the current plan is to "just" have a replacement of Qconfig.py, not supporting multiple accounts from the same provider at the moment - the idea is to have a solid and manageable foundation that supports the main use case, and in future PRs we can append more functionality and extend the support for more complex scenarios. I hope that it makes sense!

In b09e794b8b70769c5262776dffe6ac5b45f303cb some basic tests were added for the core functionality. I have taken off the on hold label and marked it again as [WIP], as there are still tweaks to the documentation to be made before completing this round of changes 🎉

diego-plan9 · 2018-07-16T12:09:50Z

Taking the PR out of WIP, after tweaking up a bit the documentation to account for the latest changes and for the comments in the initial review .

delapuente

In general, it looks OK but I strongly encourage using module + class name to identify the providers.

There are some typos we should fix before merging, though.

delapuente · 2018-07-19T15:25:43Z

README.md

-  ```python
-  APItoken = 'MY_API_TOKEN'
+3. We are now going to add the necessary credentials to QISKit. Take your token
+   from step 2, here called `MY_API_TOKEN`, and pass it to the


Non-blocking: it seems we are using soft wrapping so there is no need of manually breaking the lines. I think we should be consistent here.

Actually we are not really using soft-wrapping in most of the README.md - and technically using line wrapping is more in line with the rest of the file (if I recall correctly, we did have full hard-wrapping for the file at one point in time, but lots of changes were made that resulted in the current mix). I'd love indeed to have it fully consistent one way or the other eventually, but seems out of the scope of this PR.

delapuente · 2018-07-19T15:28:13Z

README.md


-4. Substitute `MY_API_TOKEN` with your real API Token extracted in step 2.
+   store_credentials('MY_API_TOKEN')


Should we say "you don't need to do this more than once"?

Hmm, I would expect that the reader knows that the steps in this whole section "Configure you API token" is to be done once (create account, create token, set it up, etc).

I saw this explained later, it is fine.

delapuente · 2018-07-19T15:30:14Z

README.md


-Once the `Qconfig.py` file is set up, you have to move it under the same directory/folder where your program/tutorial resides, so it can be imported and be used to authenticate with the `register()` function. For example:
+After calling `store_credentials()`, your credentials will be stored into disk.
+Once they are stored, you can automatically load and use them in your program


I would replace you can automatically load with QISKit will automatically load to highlight this is automatic.

delapuente · 2018-07-19T15:34:07Z

doc/install.rst

+And customize the following lines:
+
+* copy/paste your API token into the space between the quotation marks on the
+  first line (``APItoken = 'PUT_YOUR_API_TOKEN_HERE``).


I think a single quotation ' is missing after PUT_YOUR_API_TOKEN_HERE. Isn't it?

delapuente · 2018-07-19T15:47:42Z

qiskit/wrapper/_wrapper.py

    """
+    # Try to autodiscover credentials if not passed.
+    if not args and not kwargs and provider_class == IBMQProvider:
+        kwargs = credentials.discover_credentials().get(IBMQProvider.__name__) or {}


Can we call an identity(klass) function that encapsulates the calculus of the provider name?

delapuente · 2018-07-19T16:38:41Z

test/python/test_registration.py

+@skipIf(os.name == 'nt', 'Test not supported in Windows')
+class TestWrapperCredentuals(QiskitTestCase):
+    """Wrapper autoregistration and credentials test case."""
+    def test_autoregister_no_credentials(self):


Can you add a blank line before the function?

delapuente · 2018-07-19T16:44:49Z

test/python/test_registration.py

+            with self.assertRaises(QISKitError) as cm:
+                qiskit.wrapper.register()
+
+        self.assertIn('No IBMQ credentials found', str(cm.exception))


Non-blocking: I feel asserting on exception messages is brittle and I would prefer to provide a more specific exception class.

delapuente · 2018-07-19T16:46:09Z

test/python/test_registration.py

+        self.assertEqual(provider._proxies, {'http': 'foo'})
+
+    def test_store_credentials_overwrite(self):
+        """Test overwritind qiskitrc credentials."""


Typo: replace overwritind with overwriting.

delapuente · 2018-07-19T16:52:23Z

test/python/test_registration.py

+        self.assertEqual(provider._token, 'QISKITRC_TOKEN')
+        self.assertEqual(provider._proxies, {'http': 'foo'})
+
+    def test_store_credentials_overwrite(self):


I would break this test in several, one testing overwriting fails, another for testing failure implies no data is overwritten.

delapuente · 2018-07-19T16:55:18Z

test/python/test_registration.py

+@contextmanager
+def mock_ibmq_provider():
+    """Mock the initialization of IBMQProvider, so it does not query the api."""
+    patcher = patch.object(IBMQProvider, '_authenticate', return_value=None)


Feel free to ignore this but you can simplify this pattern with:

@contextmanager def mock_ibmq_provider(): with patch.object(IBMQProvider, '_authenticate', return_value=None), path.object(IBMQProvider, '_discover_remote_backends', return_value={}): yield

@nonhermitian

Squash of the previous commits by @nonhermitian: lint fixes Make register token a kwarg - Allows to pass **dict to register. Add load_local_credentials - Load from qiskitrc, env vars, and local qconfig.py lint fixes lint fixes again get rid of circular imports style fixes get rid of cyclic import credentials mofifiers - Added get. store, and remove credentials functons to modify local qiskitrc file. New license format everything done in register now register called with no args looks for credentials Tests now read credentials from qiskitrc make auto register play nice with no internet connection better text for auto register fail Update readme update install docs fix skip provider if already registered Test new decorator and simple test additional tests fix lint lint again fix bad test class name ignore unused argments flip decorators test register from qiskitrc pylint Update test_offline test_quantumprogram.test_offline Fails locally with this PR if the user has credentials registered as the register function will just skip the fake registration as the IBMQProvider is already registered. Thus I made the test CI only. ouput warning when attempting to double registering provider Add provider name and unregister function auto-reg with user supplied names default provider naming for IBMQ in store_credentials register single provider by name go back to old ci check minor syntax change baseprovider names base check connection in _register to exit gracefully. remove auto-register and cleanup add get_credentials test remove default Qconfig try to fix online test Better error messages when registering fix lint Updates from Ali - Add available_providers function - Rename 'local' to 'core' provider. - Make sure you cannot unregister 'core' -Remove old docs about auto_register. save_credentials when calling register. Do Diego updates fix issues with refactor Updates - Move _register to its own file to avoid circular imports. fix provider name issue comment out offending test move _DEFAULT_PROVIDER to _register Fix specific provider registration simplify tests that need clean provider list typo Remove the ability to set provider name - Also set core -> terra changes based on comments remove provider name call load test credentials from qnet

Refactor the features for reading credentials from different sources, centered in the `_configrc.py` file: * rename the module to `wrapper.credentials` for generalizing. * make the default level of granularity a dict of providers when working with qiskitrc files. * simplify the usage by writing and reading dicts of providers directly. * replace the format used by the configuration file, for simplicity: each provider account is an .ini section, with the variables listed explicitely. Revise credentials discovery from qconfig * Move the discovery of credentials to its own file. * Revise the arguments and return value for unifying it with the rest of discovery features. * Add a deprecation warning when using the `Qconfig.py`. Revise credentials discovery from environment * Refactor and move the discovery of credentials from environment variables to conform to the rest of the module changes. * Further clean the remaining methods of `_configrc.py`, making it only contain the functionality related to the config file.

Revise the wrapper `register()` functionality: * allow autoregistration for the IBMQProvider if the function is called without parameters (no args or kwargs), using the `credentials.*` methods for trying to find a working configuration. * add `store_credentials()` method, for allowing the user to store credentials in the qiskitrc file. Update the `credentials.read_credentials_from_x` to unify the return types, allowing for only one IBMQProvider credentials. Fix bug with Qconfig.config and with parsing of the proxies parameter. Revise tests, including context manager for mocking the different configuration locations, and adding tests for the autoregistration and the credentials discovery.

Fixes after review: * use the full class name (module + class) for the account name, in order to ensure it is unique, moving the calculation to its own function for reusability. * spelling and other fixes.

diego-plan9 · 2018-07-20T09:51:12Z

Thanks for the review, @delapuente ! I have addressed most of the concerns, focusing on the main one (the use of the full qualified name of the class as the account name) and the unusual amount of typos, leaving some of the comments for future iterations.

delapuente

Hi Diego, it looks great to me. Just the comment about the error but it's non-blocking. Nice work.

delapuente · 2018-07-20T10:06:42Z

README.md


-4. Substitute `MY_API_TOKEN` with your real API Token extracted in step 2.
+   store_credentials('MY_API_TOKEN')


I saw this explained later, it is fine.

delapuente · 2018-07-20T10:15:44Z

test/python/common.py

-        kwargs['group'] = QE_GROUP
-        kwargs['project'] = QE_PROJECT
+        # Cleanup the credentials, as this file is shared by the tests.
+        from qiskit.wrapper import _wrapper


Non-blocking: We should not have production code depending on tests, I've opened #669 for addressing this.

delapuente · 2018-07-20T10:16:44Z

test/python/common.py

+                'project': credentials.get('project'),
+            })
+        else:
+            raise Exception('Could not locate valid credentials')


Should not it be a QISKitError?

Hmm, I don't think so, since this exception raised in the test suite (actually before the tests), so the usual conventions for the qiskit module do not really apply.

Improve user authentication handling

nonhermitian requested review from ajavadia, atilag, diego-plan9 and ewinston as code owners June 7, 2018 16:55

ewinston reviewed Jun 7, 2018

View reviewed changes

nonhermitian requested a review from jaygambetta as a code owner June 8, 2018 19:22

nonhermitian changed the title ~~WIP: Improve user authentication handling~~ Improve user authentication handling Jun 11, 2018

ajavadia reviewed Jun 12, 2018

View reviewed changes

diego-plan9 reviewed Jun 12, 2018

View reviewed changes

nonhermitian mentioned this pull request Jun 15, 2018

Avoid double registration of same provider/credentials #568

Closed

delapuente reviewed Jun 18, 2018

View reviewed changes

nonhermitian commented Jun 18, 2018

View reviewed changes

delapuente suggested changes Jun 19, 2018

View reviewed changes

diego-plan9 added this to the 0.6 milestone Jun 20, 2018

diego-plan9 added the on hold Can not fix yet label Jun 21, 2018

diego-plan9 removed the on hold Can not fix yet label Jul 10, 2018

diego-plan9 changed the title ~~Improve user authentication handling~~ [WIP] Improve user authentication handling Jul 10, 2018

delapuente assigned diego-plan9 Jul 11, 2018

diego-plan9 changed the title ~~[WIP] Improve user authentication handling~~ Improve user authentication handling Jul 16, 2018

delapuente self-assigned this Jul 19, 2018

delapuente reviewed Jul 19, 2018

View reviewed changes

nonhermitian and others added 6 commits July 20, 2018 11:34

Update documentation for auto registration

7d30b90

Add changelog entry

8e1ca57

Use full class name for account name, spelling

46286a6

Fixes after review: * use the full class name (module + class) for the account name, in order to ensure it is unique, moving the calculation to its own function for reusability. * spelling and other fixes.

diego-plan9 approved these changes Jul 20, 2018

View reviewed changes

delapuente approved these changes Jul 20, 2018

View reviewed changes

diego-plan9 merged commit 42a5744 into Qiskit:master Jul 20, 2018

diego-plan9 mentioned this pull request Jul 23, 2018

Improve user authentication handling (QConfig) #540

Closed

4 tasks

nonhermitian deleted the configrc branch August 21, 2018 17:00

lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this pull request Jul 30, 2019

Merge pull request Qiskit#547 from nonhermitian/configrc

fd7bdbe

Improve user authentication handling


		4. Substitute `MY_API_TOKEN` with your real API Token extracted in step 2.
		store_credentials('MY_API_TOKEN')

Improve user authentication handling #547

Improve user authentication handling #547

Conversation

nonhermitian commented Jun 7, 2018 • edited by delapuente Loading

Description

Summary of Changes Made

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonhermitian commented Jun 7, 2018

diego-plan9 commented Jun 8, 2018 • edited Loading

nonhermitian commented Jun 8, 2018

nonhermitian commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diego-plan9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delapuente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delapuente left a comment

nonhermitian commented Jun 7, 2018 •

edited by delapuente

Loading

diego-plan9 commented Jun 8, 2018 •

edited

Loading