Add unregister() via DefaultQISKitProvider changes #584

diego-plan9 · 2018-06-20T20:20:56Z

Summary

Add wrapper.unregister() for removing previously registered providers,
by delegating into DefaultQISKitProvider internal tracking of
provider instances:

use a dict instead of a list for self.providers, introducing the
ability to assign a user name to a provider instance at the wrapper
level.
update register(...,provider_name) parameter so it is set by the
user explicitly (with "weak" fallbacks for the two main use cases,
for backwards compatibility).
add wrapper.registered_providers() for convenience, displaying the
list of currently registered providers.

Regroup wrapper tests, adding basic ones for the new functionality and
fixing an issue with global variables and imports via tearDown.

Details and comments

Even if a related functionality was included in #540, after discussion with Jay I'm moving this feature to its own PR for being able to incorporate it into the codebase independently and hopefully cater to the urgency, as #540 is ongoing a refactoring and has a wider scope. The modifications for DefaultQiskitProvider are actually part of that refactor - once this PR is merged, changes will be moved onto the other pull request.

Add `wrapper.unregister()` for removing previously registered providers, by delegating into `DefaultQISKitProvider` internal tracking of provider instances: * use a dict instead of a list for `self.providers`, introducing the ability to assign a user name to a provider instance at the wrapper level. * update `register(...,provider_name)` parameter so it is set by the user explicitly (with "weak" fallbacks for the two main use cases, for backwards compatibility). * add `wrapper.registered_providers()` for convenience, displaying the list of currently registered providers. Regroup wrapper tests, adding basic ones for the new functionality and fixing an issue with global variables and imports via `tearDown`.

delapuente · 2018-06-21T07:25:45Z

qiskit/wrapper/_wrapper.py

@@ -38,17 +37,33 @@ def register(token, url='https://quantumexperience.ng.bluemix.net/api',
            proxies (dict): Proxy configuration for the API, as a dict with
                'urls' and credential keys.
            verify (bool): If False, ignores SSL certificates errors.
-            provider_name (str): the unique name for the online backend
-                provider (for example, 'ibmq' for the IBM Quantum Experience).
+            provider_name (str): the user name for the registered provider.


Can you replace the user name for with a user-provided name for.

delapuente · 2018-06-21T07:28:41Z

qiskit/wrapper/defaultqiskitprovider.py

        """
        Add a new provider to the list of known providers.

        Args:
            provider (BaseProvider): Provider instance.
+            provider_name (str): User name for the provider.


Again, replace User name for with a user-provided name for.

delapuente · 2018-06-21T07:29:05Z

qiskit/wrapper/defaultqiskitprovider.py

+
+        Args:
+            credentials_dict (dict): dictionary of credentials for a provider.
+            provider_name (str): User name for the provider. A name will


delapuente · 2018-06-21T07:29:34Z

qiskit/wrapper/defaultqiskitprovider.py

+        Args:
+            credentials_dict (dict): dictionary of credentials for a provider.
+            provider_name (str): User name for the provider. A name will
+                automatically


I think this is an incomplete sentence.

Yep 🤦‍♂️

delapuente · 2018-06-21T11:31:43Z

qiskit/wrapper/defaultqiskitprovider.py

+                'A provider with name "%s" is already registered.'
+                % provider_name)
+
+        self.providers[provider_name] = provider


Non-blocking: should we return the provider to keep it consistent with add_ibmq_provider?

Sure, makes sense!

delapuente · 2018-06-21T14:45:47Z

test/python/test_wrapper.py

+
+    def test_register_unknown_name(self):
+        """Test registering a provider with not explicit name."""
+        with self.assertRaises(QISKitError):


Non-blocking: I'd relax this constraint by asserting the list before and after the intent to register are equal:

before_attempt = qiskit.wrapper.registered_providers() with self.assertRaises(QISKitError): qiskit.wtapper.register('FAKE_TOKEN', 'http://unknown') self.assertEqual(sorted(qiskit.wrapper.registered_providers()), sorted(before_attempt))

This way you're checking that a failing operation does not alter the list of providers, instead of tying yourself to a particular implementation such as the 'local' name.

delapuente · 2018-06-21T14:47:53Z

test/python/test_wrapper.py

+    @requires_qe_access
+    def test_unregister(self, QE_TOKEN, QE_URL, hub, group, project):
+        """Test unregistering."""
+        qiskit.wrapper.register(QE_TOKEN, QE_URL, hub, group, project,


Same here, we could relax the test by replacing the last assertion with a comparison between the current registered_providers and those before registering anything and...

delapuente · 2018-06-21T14:48:28Z

test/python/test_wrapper.py

+        """Test unregistering."""
+        qiskit.wrapper.register(QE_TOKEN, QE_URL, hub, group, project,
+                                provider_name='provider1')
+        self.assertEqual(['local', 'provider1'],


...replacing this one with an inclusion assertion.

delapuente · 2018-06-21T14:48:55Z

test/python/test_wrapper.py

+
+    def test_unregister_non_existent(self):
+        """Test unregistering a non existent provider."""
+        with self.assertRaises(QISKitError):


delapuente · 2018-06-21T14:52:26Z

CHANGELOG.rst

@@ -20,6 +20,7 @@ The format is based on `Keep a Changelog`_.

 Added
 -----
+- Add ``unregister()`` for removing previously registered providers (#584).


If changing the signatures or register() and unregister() for them to return the provider instances, consider adding that here too.

* Revise docstrings with reference to user-provided name. * Make tests more robust. * Misc fixes.

diego-plan9 · 2018-06-21T16:12:34Z

Thanks for the review, @delapuente ! I have addressed most of the issues, keeping some of them still open with the idea of further discussing and addressing them when #540 is tackled while still providing a functional unregister().

delapuente

LGTM, let's discuss the non-blocking matters after getting more insight about the original PR.

…/unregister

Add a check during backend registration that prints a warning if a backend name clash is detected.

…/unregister

delapuente · 2018-06-27T19:38:49Z

qiskit/wrapper/defaultqiskitprovider.py

@@ -103,6 +110,16 @@ def add_provider(self, provider, provider_name):
                'A provider with name "%s" is already registered.'
                % provider_name)

+        # Check for backend name clashes, emitting a warning.
+        current_backends = {str(backend) for backend in self.available_backends()}
+        added_backends = {str(backend) for backend in provider.available_backends()}


Non-blocking: can we make it clear with the name that we did not add the backends yet? That they are incoming_backends or new_provider_backends or something like this?

delapuente · 2018-06-27T19:44:06Z

qiskit/wrapper/defaultqiskitprovider.py

@@ -87,6 +88,12 @@ def add_provider(self, provider, provider_name):
        """
        Add a new provider to the list of known providers.

+        Note:
+            If a backend from the new provider has a name that is in use by an


Non-blocking but what about: If some backend of the new provider has a name in use by an already registered provider, the backend will not be available, and the name will always refer to that previously registered.

delapuente · 2018-06-27T19:44:51Z

qiskit/wrapper/defaultqiskitprovider.py

+        if common_backends:
+            logger.warning(
+                'The backend names "%s" (provided by "%s") are already in use. '
+                'Consider using unregister() for avoiding namespace conflicts.',


Non-blocking but I would seriously consider changing namespace with simply name.

delapuente · 2018-06-27T19:48:49Z

test/python/test_wrapper.py

+                                provider_name='provider1')
+        initial_providers = registered_providers()
+        initial_backends = qiskit.wrapper.available_backends()
+        with self.assertLogs(level=logging.WARNING) as context:


Can we rename context with logger_output or simply output? Does it make sense?

delapuente · 2018-06-27T19:59:03Z

test/python/test_wrapper.py

+        self.assertCountEqual(initial_providers + ['provider2'],
+                              registered_providers())
+        # Check that no new backends have been added.
+        self.assertCountEqual(initial_backends,


We should also assert that the backend name still refers to the already registered backend:

initial_backends = qiskit.wrapper.available_backends() original_backend = qiskit.wrapper.get_backend('ibmqx4') # ... with self.assertLogs(level=logging.WARNING) as context: qiskit.wrapper.register(QE_TOKEN, QE_URL, hub, group, project, provider_name='provider2') # ... self.assertEqual(original_backend, qiskit.wrapper.get_backend('ibmqx4'))

Test now check if, after registering a provider with colliding backend names, an specific backend name still refers to that available before registering the provider.

* Add unregister() via DefaultQISKitProvider changes Add `wrapper.unregister()` for removing previously registered providers, by delegating into `DefaultQISKitProvider` internal tracking of provider instances: * use a dict instead of a list for `self.providers`, introducing the ability to assign a user name to a provider instance at the wrapper level. * update `register(...,provider_name)` parameter so it is set by the user explicitly (with "weak" fallbacks for the two main use cases, for backwards compatibility). * add `wrapper.registered_providers()` for convenience, displaying the list of currently registered providers. Regroup wrapper tests, adding basic ones for the new functionality and fixing an issue with global variables and imports via `tearDown`. * Fixes wrapper unregister and tests after review * Revise docstrings with reference to user-provided name. * Make tests more robust. * Misc fixes. * Add warning if backend name clash when registering Add a check during backend registration that prints a warning if a backend name clash is detected. * Polishing names and improving tests. Test now check if, after registering a provider with colliding backend names, an specific backend name still refers to that available before registering the provider.

diego-plan9 requested review from ajavadia, atilag, delapuente and ewinston as code owners June 20, 2018 20:20

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

diego-plan9 added the priority: high label Jun 20, 2018

diego-plan9 force-pushed the feature/unregister branch from 2450fd2 to bfd4ea9 Compare June 20, 2018 20:50

diego-plan9 mentioned this pull request Jun 21, 2018

Improve user authentication handling #547

Merged

14 tasks

delapuente reviewed Jun 21, 2018

View reviewed changes

diego-plan9 added 2 commits June 21, 2018 17:32

Fixes wrapper unregister and tests after review

e3d3e26

* Revise docstrings with reference to user-provided name. * Make tests more robust. * Misc fixes.

Merge remote-tracking branch 'qiskit/master' into feature/unregister

e05068e

Merge branch 'master' into feature/unregister

a5fbf93

delapuente previously approved these changes Jun 21, 2018

View reviewed changes

diego-plan9 added 2 commits June 27, 2018 15:41

Merge branch 'master' of github.com:QISKit/qiskit-sdk-py into feature…

1fbb1a4

…/unregister

Add warning if backend name clash when registering

85e6395

Add a check during backend registration that prints a warning if a backend name clash is detected.

diego-plan9 dismissed delapuente’s stale review via 28449c5 June 27, 2018 19:03

Merge branch 'master' of github.com:QISKit/qiskit-sdk-py into feature…

cec2fdf

…/unregister

diego-plan9 force-pushed the feature/unregister branch from 28449c5 to cec2fdf Compare June 27, 2018 19:05

delapuente suggested changes Jun 27, 2018

View reviewed changes

Polishing names and improving tests.

d62d938

Test now check if, after registering a provider with colliding backend names, an specific backend name still refers to that available before registering the provider.

delapuente force-pushed the feature/unregister branch from eb0c355 to d62d938 Compare June 27, 2018 20:30

delapuente modified the milestones: 0.6, 0.5.5 Jun 27, 2018

delapuente approved these changes Jun 27, 2018

View reviewed changes

diego-plan9 merged commit e51b4fd into Qiskit:master Jun 27, 2018

diego-plan9 deleted the feature/unregister branch June 28, 2018 19:26

ajavadia mentioned this pull request Jun 29, 2018

Enable registration of add-on providers #602

Merged

diego-plan9 mentioned this pull request Jul 2, 2018

backend naming issue when registering new provider #529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unregister() via DefaultQISKitProvider changes #584

Add unregister() via DefaultQISKitProvider changes #584

diego-plan9 commented Jun 20, 2018

delapuente Jun 21, 2018

delapuente Jun 21, 2018

delapuente Jun 21, 2018

delapuente Jun 21, 2018

diego-plan9 Jun 21, 2018

delapuente Jun 21, 2018

diego-plan9 Jun 21, 2018

delapuente Jun 21, 2018

delapuente Jun 21, 2018

delapuente Jun 21, 2018

delapuente Jun 21, 2018

delapuente Jun 21, 2018

diego-plan9 commented Jun 21, 2018

delapuente left a comment

delapuente Jun 27, 2018

delapuente Jun 27, 2018

delapuente Jun 27, 2018

delapuente Jun 27, 2018

delapuente Jun 27, 2018

Add unregister() via DefaultQISKitProvider changes #584

Add unregister() via DefaultQISKitProvider changes #584

Conversation

diego-plan9 commented Jun 20, 2018

Summary

Details and comments

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

diego-plan9 commented Jun 21, 2018

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