Repository: astropy/pyvo
Pull request #309 (2022-03-11 14:58:28)

support decorating experimental features.
Description
-------------

Add support for tagging prototype features.

Note I tweaked the use cases as per comments but left the old wording for clarity, while I just edited the rest of the description.

To Do
------

- [x] Change "experimental" -> "prototype"
- [x] instantiate and register features statically
- [x] simplify decorator to only accept (and require) the prototype tag
- [x] make prototype features disabled by default (error out if they are used but not enabled, remove warnings)
- [x] update PR description
- [x] update docs

Use Cases
-----------

As a pyvo developer of a service client I want to support features that are not yet standard. Features may include several functions, and potentially all the public methods of a new class.

~~As a pyvo developer of a service client I want to properly warn `pyvo` clients that the features may change in the future.~~

As a pyvo client I want ~~to be warned about the experimental nature of the feature with a warning~~ pyvo to prevent me from inadvertently calling a prototype feature.

As a pyvo client I want to be able to ~~completely turn off~~ activate one, several, or all experimental features, so the system *does not* error~~s~~ out when an experimental feature is accessed, either in a script or in an interactive session.

~~As a pyvo client I want to turn off warnings for one, several, or all experimental features, so the system does not produce any warnings in my interactive session or scripts.~~

Note that I am not covering re-enabling features. I couldn't think of how it could be useful, unless maybe pyvo is used to back a web application, in which case the settings might be turned on and off more than once, or user-specific feature settings may need to be serialized and then rehydrated. I don't think pyvo has a configuration file, but if there was, we could support that kind of pattern.

Design
--------

The design revolves around the definition of a `Feature` class in the new `prototype` package. It defines a prototype feature which may contain multiple classes and functions. Features have a name, an optional documentation URL, and information on whether they are shut down (they will error out if used).

The `Feature` class could be extended for use cases that might arise in the future without having to change the client code, because the class is responsible for its own validation.

Features are statically defined in a dictionary in the `prototype` module.

### Pyvo developer user stories

A natural way of designing the interface to tag prototype features seems to be decorators. Decorators are declarative, which is a nice and concise way of tagging functions and classes. The design restricts the possible usage of the decorator, which needs to always be called with a single argument being the name of the corresponding feature. More arguments are allowed but will be ignored. If the decorator is not used with the correct `prototype_feature("feature-name")` invocation, the code will error as soon as the class is imported, which should be picked up by CI (I'm assuming new features are unit-tested ;) ).

We can have:

```python
    @prototype_feature('my-feature')
    def i_am_experimental(arg):
        arg('called')
```

or

```python
    @prototype_feature('class')
    class Feature:
        def method(self):
            pass

        @staticmethod
        def static():
            pass

        def __ignore__(self):
            pass
```

and anything in between, like decorating individual methods in a class, but not the entire class.

Features can be initialized with a URL that will be included in the error message where users can find more information. If the URL is not specified, then of course the "for more details" message is not rendered.

Developers have two options: decorate single functions/methods, or decorate an entire class. If they decorate the class, then all "public" methods, static or otherwise, are decorated dynamically. Note that there is no way of excluding some of the methods if they decorate the class, which seems a reasonable constraint.

I decided to go with a Singleton pattern, in a Pythonic way, by making the feature registry a module level variable. That poses challenges for unit tests, which I tried to address in the setup fixture. However it seems safer and simpler than trying to find a place to instantiate and inject the registry and enforce there's only one instance clients can access during a session. We can complicate that if necessary.

### pyvo client user stories

If the client doesn't activate features, calling them will result in an error.

Note that the error is triggered when the methods are called, and not when the class is instantiated. I think that's reasonable because the class might be instantiated by some mediator before the user can call (and more importantly not call) a higher level facade.

Clients can opt-in features through the `activate_features` function. The function accepts a variable number of strings as arguments, one per feature name, and they act on entire features, not single functions or classes. If no arguments are provided, the function activates the whole list of features.

### Error handling

If the decorator is not properly used, it will generate an error.

For clients, I think the main error case is trying to activate a non-existent feature. In that case, the system emits a warning.


codecov[bot]: # [Codecov](https://codecov.io/gh/astropy/pyvo/pull/309?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=astropy) Report
> Merging [#309](https://codecov.io/gh/astropy/pyvo/pull/309?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=astropy) (b118e34) into [main](https://codecov.io/gh/astropy/pyvo/commit/9d5c501bbe3b9b80c2db17415a65a2ebd9559dad?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=astropy) (9d5c501) will **increase** coverage by `0.34%`.
> The diff coverage is `98.71%`.

```diff
@@            Coverage Diff             @@
##             main     #309      +/-   ##
==========================================
+ Coverage   75.49%   75.84%   +0.34%     
==========================================
  Files          44       45       +1     
  Lines        5125     5203      +78     
==========================================
+ Hits         3869     3946      +77     
- Misses       1256     1257       +1     
```


| [Impacted Files](https://codecov.io/gh/astropy/pyvo/pull/309?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=astropy) | Coverage Δ | |
|---|---|---|
| [pyvo/utils/experimental.py](https://codecov.io/gh/astropy/pyvo/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=astropy#diff-cHl2by91dGlscy9leHBlcmltZW50YWwucHk=) | `98.70% <98.70%> (ø)` | |
| [pyvo/utils/\_\_init\_\_.py](https://codecov.io/gh/astropy/pyvo/pull/309/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=astropy#diff-cHl2by91dGlscy9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | |

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=astropy)

eerovaher: Wouldn't it be better to implement this in the `astropy` core package?
andamian: Thanks for taking the lead on this @olaurino - this is great!

I know that we've called the feature `experimental` when we've talked about it but now I'm wondering whether it's more appropriate to call it `prototype`. `PyVO` implements the IVOA specs (BTW - the standard numbers should probably be listed with the version) but sometimes new features are proposed and need to be demonstrated first (such as https://github.com/astropy/pyvo/pull/274). They are added before they are stable (new spec approved) but the expectation is that they will eventually become standard at which time the decorator could be removed. I don't think we should have any arbitrary feature even if it's flagged as `experimental`. I see `prototype` as the opposite of `deprecated`. That's my opinion.

Otherwise, I agree with the requirements listed above. The default should probably `no experimental`.

I will take a closer look at the code in the coming days.
bsipocz: @eerovaher - I think this very much belongs to pyvo, it's specific to the needs here (note: these are experimental/prototype features in the context of accepted VO standards and not about decorating hacky, experimental code). So I think these shouldn't be upstreamed until there is a large, concrete interest in them from other libraries.
olaurino: @andamian if we go with the opt-in approach then I guess warnings aren't necessary anymore? prototype functions either error out or they don't if their feature is switched on..

Also, one thing I should have stressed is that features are dynamically generated as the interpreter imports decorated functions. However we could have tighter control on the prototypes by explicitly declaring features in the registry and then the decorator would only link the function class to an existing feature. Then tagging a function/class with a non-existent feature name is a mistake.

There may be a case for supporting experimental features, i.e. features that are not related to a standard but built on top of the standards themselves and are not yet stable. Even the Linux kernel supports "experimental" features. Doesn't mean they are "hacky".
olaurino: I should be able to make changes based on @andamian's feedback later this week.

Patch:
diff --git a/docs/index.rst b/docs/index.rst
index a440e8b..4350e58 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -122,3 +122,4 @@ Using `pyvo`
    registry/index
    io/index
    auth/index
+   prototypes/index
diff --git a/docs/prototypes/index.rst b/docs/prototypes/index.rst
new file mode 100644
index 0000000..4bafbb7
--- /dev/null
+++ b/docs/prototypes/index.rst
@@ -0,0 +1,89 @@
+.. _pyvo-prototypes:
+
+**************************************************
+Prototype Implementations (`pyvo.utils.prototype`)
+**************************************************
+
+This subpackage provides support for prototype standard implementations.
+
+``PyVO`` implements the IVOA standards. As part of the standard approval process, new features
+are proposed and need to be demonstrated before the standard may be approved. ``PyVO``
+may implement features that are not yet part of an approved standard.
+Such features are unstable, as the standard may be subject to reviews and significant changes, until it's finally
+approved.
+
+The ``prototype`` package provides support for such prototypes by means of a decorator
+for implementations that are still unstable. The expectation is that they will eventually become
+standard at which time the decorator will be removed.
+
+Users of ``pyvo`` need to explicitly opt-in in order to use such features. If prototype
+implementations are accessed without the user explicitly opting in, an exception will be raised.
+
+.. _pyvo-prototypes-users:
+
+Activating Prototype Implementations
+====================================
+
+In order to activate a feature, users need to call the function::
+
+   activate_features('feature_one', 'feature_two')
+
+Where the arguments are names of prototype features. If a feature name does not exist, a `~pyvo.utils.prototype.PrototypeWarning`
+will be issued, but the call will not fail. If no arguments are provided, then all features are enabled.
+
+.. _pyvo-prototypes-developers:
+
+Marking Features as Experimental
+================================
+The design restricts the possible usage of the decorator, which needs to always be called
+with a single argument being the name of the corresponding feature. More arguments are allowed
+but will be ignored. If the decorator is not used with the correct
+``@prototype_feature("feature-name")`` invocation, the code will error as soon as the class is
+imported.
+
+The decorator can be used to tag individual functions or methods::
+
+   @prototype_feature('a-feature')
+   def i_am_a_prototype(*arg, **kwargs):
+      pass
+
+In this case, a single function or method is tagged as part of the ``a-feature`` prototype feature. If the feature
+has a URL defined (see :ref:`pyvo-prototypes-registry` below).
+
+Alternatively, a class can be marked as belonging to a feature. All public methods will be marked as part of the
+prototype implementation. Protected, private, and *dunder* methods (i.e. any method starting with
+an underscore) will be ignored. The reason is that the class might be instantiated by some mediator before the
+user can call (and more importantly not call) a higher level facade::
+
+   @prototype_feature('a-feature')
+   class SomeFeatureClass:
+       def method(self):
+           pass
+
+       @staticmethod
+       def static():
+           pass
+
+       def __ignore__(self):
+           pass
+
+Any number of classes and functions can belong to a single feature, and individual methods can be tagged
+in a class rather than the class itself.
+
+.. _pyvo-prototypes-registry:
+
+Feature Registry
+================
+
+The feature registry is a static ``features`` dictionary in the `~pyvo.utils.prototype` package. The key is the name
+of the feature and the value is an instance of the `~pyvo.utils.prototype.Feature` class. This class is responsible for determining
+whether an instance should error or not, and to format an error message if it's not. While the current implementation
+of the ``Feature`` class is simple, future requirements might lead to other implementations with more complex logic or
+additional documentation elements.
+
+.. _pyvo-prototypes-api:
+
+Reference/API
+=============
+
+.. automodapi:: pyvo.utils.prototype
diff --git a/pyvo/utils/__init__.py b/pyvo/utils/__init__.py
index 9fee358..96be2b9 100644
--- a/pyvo/utils/__init__.py
+++ b/pyvo/utils/__init__.py
@@ -1,1 +1,2 @@
 from .compat import *
+from .prototype import prototype_feature, activate_features
diff --git a/pyvo/utils/prototype.py b/pyvo/utils/prototype.py
new file mode 100644
index 0000000..55899f1
--- /dev/null
+++ b/pyvo/utils/prototype.py
@@ -0,0 +1,169 @@
+import inspect
+import warnings
+from dataclasses import dataclass
+from functools import wraps
+from typing import Dict, Iterable
+
+from pyvo.dal.exceptions import PyvoUserWarning
+
+__all__ = ['features', 'prototype_feature', 'activate_features', 'Feature', 'PrototypeWarning', 'PrototypeError']
+
+features: Dict[str, "Feature"] = {
+
+}
+
+
+def prototype_feature(*args):
+    """
+    Decorator for functions and classes that implement unstable standards which haven't been approved yet.
+    The decorator can be used to tag individual functions or methods.
+
+    Please refer to the user documentation for details.
+
+    Parameters
+    ----------
+    args: iterable of arguments.
+        Currently, the decorator must always be called with one and only one argument, a string representing
+        the feature's name associated with the decorated class or functions. Additional arguments will be ignored,
+        while using the decorator without any arguments will result in a ``PrototypeError`` error.
+
+    Returns
+    -------
+    The class or function it decorates, which will be associated to the feature provided as argument.
+
+    """
+    feature_name = _parse_args(*args)
+    decorator = _make_decorator(feature_name)
+    return decorator
+
+
+def activate_features(*feature_names: Iterable[str]):
+    """
+    Activate one or more prototype features.
+
+    Parameters
+    ----------
+    feature_names: Iterable[str]
+        An arbitrary number of feature names. If a feature with that name does not exist, a `PrototypeWarning` will
+        be issued. If no arguments are provided, all features will be activated
+
+    Returns
+    -------
+
+    """
+    names = feature_names or set(features.keys())
+    for name in names:
+        if not _validate(name):
+            continue
+        features[name].on = True
+
+
+@dataclass
+class Feature:
+    """
+    A prototype feature implementing a standard that is currently in the process of being approved, but that might
+    change as a result of the approval process. A Feature must have a name. Optionally, a feature may have a *url*
+    that is displayed to the user in case a feature is used without the user explicitly opting in on its usage.
+    The URL is expected to contain more information about the standard and its state in the approval process.
+    """
+    name: str
+    url: str = ''
+    on: bool = False
+
+    def should_error(self):
+        """
+        Should accessing this feature fail?
+
+        Returns
+        -------
+        bool Whether accessing this feature should result in an error.
+        """
+        return not self.on
+
+    def error(self, function_name):
+        """
+        Format an error message when the feature is being accesses without the user having opted in its usage.
+
+        This function will be used as a callback when an error message needs to be displayed to the user, with the
+        function name that was accessed as an argument. Extensions of this class may have additional information to
+        display.
+
+        Parameters
+        ----------
+        function_name: str
+            The name of the function associated to this feature and that the user called.
+
+        Returns
+        -------
+        str: The error message to be displayed to the user.
+
+        """
+        message = f'{function_name} is part of a prototype feature ({self.name}) that has not been activated. ' \
+                  f'For information about prototype features please refer to ' \
+                  f'https://pyvo.readthedocs.io/en/latest/prototypes .'
+        if self.url:
+            message += f' For more information about the {self.name} feature please visit {self.url}.'
+        message += f' To suppress this error and enable the feature use `pyvo.utils.activate_features(\'{self.name}\')`'
+        return message
+
+
+class PrototypeError(Exception):
+    pass
+
+
+class PrototypeWarning(PyvoUserWarning):
+    pass
+
+
+def _parse_args(*args):
+    if not args or callable(args[0]):
+        raise PrototypeError("The `prototype_feature` decorator must always be called with the feature name as an "
+                             "argument")
+    return args[0]
+
+
+def _make_decorator(feature_name):
+
+    def decorator(decorated):
+        if inspect.isfunction(decorated):
+            return _make_wrapper(feature_name, decorated)
+
+        if inspect.isclass(decorated):
+            method_infos = inspect.getmembers(decorated, predicate=_should_wrap)
+            _wrap_class_methods(decorated, method_infos, feature_name)
+
+        return decorated
+
+    return decorator
+
+
+def _validate(feature_name):
+    if feature_name not in features:
+        warnings.warn(f'No such feature "{feature_name}"', category=PrototypeWarning)
+        return False
+    return True
+
+
+def _warn_or_raise(function, feature_name):
+    _validate(feature_name)
+    feature = features[feature_name]
+
+    if feature.should_error():
+        raise PrototypeError(feature.error(function.__name__))
+
+
+def _should_wrap(member):
+    return inspect.isfunction(member) and not member.__name__.startswith('_')
+
+
+def _wrap_class_methods(decorated_class, method_infos, feature_name):
+    for method_info in method_infos:
+        setattr(decorated_class, method_info[0], _make_wrapper(feature_name, method_info[1]))
+
+
+def _make_wrapper(feature_name, function):
+    @wraps(function)
+    def wrapper(*args, **kwargs):
+        _warn_or_raise(function, feature_name)
+        return function(*args, **kwargs)
+    return wrapper


Test patch:
diff --git a/pyvo/utils/tests/test_prototype.py b/pyvo/utils/tests/test_prototype.py
new file mode 100644
index 0000000..eb72839
--- /dev/null
+++ b/pyvo/utils/tests/test_prototype.py
@@ -0,0 +1,151 @@
+import warnings
+from copy import deepcopy
+from unittest import mock
+
+import pytest
+
+from pyvo.utils import prototype_feature, activate_features, prototype
+from pyvo.utils.prototype import PrototypeError, PrototypeWarning, Feature
+
+
+@pytest.fixture
+def prototype_function(features):
+    features({
+        'my-feature': Feature('my-feature', url='http://somewhere/else')
+    })
+
+    @prototype_feature('my-feature')
+    def i_am_prototype(arg):
+        arg('called')
+
+    return i_am_prototype
+
+
+@pytest.fixture
+def features():
+    previous_available = deepcopy(prototype.features)
+    prototype.features.clear()
+
+    def add(features):
+        prototype.features.update(features)
+
+    yield add
+
+    prototype.features.clear()
+    prototype.features.update(previous_available)
+
+
+def test_feature_turned_off_by_default(prototype_function):
+    with pytest.raises(PrototypeError) as e:
+        prototype_function(None)
+
+    assert 'i_am_prototype is part of a prototype feature (my-feature) that has not been activated. ' in str(e.value)
+    assert 'please visit http://somewhere/else' in str(e.value)
+    assert 'https://pyvo.readthedocs.io/en/latest/prototypes' in str(e.value)
+    assert "pyvo.utils.activate_features('my-feature')" in str(e.value)
+
+
+def test_activate_feature(prototype_function):
+    probe = mock.Mock()
+
+    activate_features('my-feature')
+
+    try:
+        prototype_function(probe)
+    except Exception as exc:
+        assert False, f"Should not have raised {exc}"
+
+    probe.assert_called_once_with('called')
+
+
+def test_non_existent_feature_warning():
+    with pytest.warns(PrototypeWarning) as w:
+        activate_features('i dont exist')
+
+    assert len(w) == 1
+    assert str(w[0].message) == 'No such feature "i dont exist"'
+
+
+def test_activate_all_features(features):
+    features({
+        'feat-one': Feature('feat-one'),
+        'feat-two': Feature('feat-two')
+    })
+
+    activate_features()
+
+    assert set(prototype.features.keys()) == {'feat-one', 'feat-two'}
+    assert prototype.features['feat-one'].on
+    assert prototype.features['feat-two'].on
+
+
+def test_decorate_class(features, recwarn):
+    features({
+        'class': Feature('class')
+    })
+    probe = mock.Mock()
+
+    @prototype_feature('class')
+    class SomePrototype:
+        def method(self):
+            probe('method')
+
+        @staticmethod
+        def static():
+            probe('static')
+
+        def __ignore__(self):
+            probe('ignore')
+
+    with pytest.raises(PrototypeError):
+        SomePrototype.static()
+
+    with pytest.raises(PrototypeError):
+        SomePrototype().method()
+
+    SomePrototype().__ignore__()
+    probe.assert_called_once_with('ignore')
+    probe.reset_mock()
+
+    activate_features('class')
+
+    SomePrototype.static()
+    probe.assert_called_once_with('static')
+    probe.reset_mock()
+
+    SomePrototype().method()
+    probe.assert_called_once_with('method')
+    probe.reset_mock()
+
+    SomePrototype().__ignore__()
+    probe.assert_called_once_with('ignore')
+
+
+def test_decorator_without_args_errors_out():
+    with pytest.raises(PrototypeError) as e:
+        @prototype_feature
+        def function():
+            pass
+
+    assert str(e.value) == "The `prototype_feature` decorator must always be called with the feature name as an " \
+                           "argument"
+
+
+def test_decorator_without_args_around_class():
+    with pytest.raises(PrototypeError) as e:
+        @prototype_feature
+        class Class:
+            pass
+
+    assert str(e.value) == "The `prototype_feature` decorator must always be called with the feature name as an " \
+                           "argument"
+
+
+def test_decorator_with_no_arguments_and_class():
+    with pytest.raises(PrototypeError) as e:
+        @prototype_feature()
+        class Class:
+            pass
+
+    assert str(e.value) == "The `prototype_feature` decorator must always be called with the feature name as an " \
+                           "argument"