-
-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support decorating experimental features. #309
Conversation
Codecov Report
@@ Coverage Diff @@
## main #309 +/- ##
==========================================
+ Coverage 75.49% 75.85% +0.35%
==========================================
Files 44 45 +1
Lines 5125 5201 +76
==========================================
+ Hits 3869 3945 +76
Misses 1256 1256
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Wouldn't it be better to implement this in the |
pyvo/utils/experimental.py
Outdated
|
||
|
||
def _warn_or_raise(function, feature_name): | ||
_validate(feature_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized this check is useless. This is a protected function called by the wrapper at run-time, when the function is invoked. That means feature_name
is defined. And in fact in the rest of the function I assumed it was.
Thanks for taking the lead on this @olaurino - this is great! I know that we've called the feature Otherwise, I agree with the requirements listed above. The default should probably I will take a closer look at the code in the coming days. |
@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. |
@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". |
I should be able to make changes based on @andamian's feedback later this week. |
b118e34
to
7dbc779
Compare
How should I proceed with documentation? I'd like to get another round of feedback to make sure this is what we want before adding proper docs, but some guidance on what to include where would be useful as this is my first contribution. |
@olaurino - we've had a |
@olaurino - I've sent you a direct message on |
Also, include url in error message, if any.
If you agree, I'd change the unit tests to include two different features, one that needs to be opted-in, the other that can be opted-out. However, as I mentioned at the end of the Apps session, I am starting to think that having to opt-out is unsafe anyway. Clients will have to know that even though it's unlikely, the API might change, even ever so slightly. So even though they won't need to opt-in, they would need to make sure they plan for a change of API, which translates to a de-facto opt-in anyway. |
Thanks. I'll have a look. I still need to find a good case for the opt-out scenario. Maybe this:
That would maybe work although it's not that different from the existing I would say we should wait until someone comes with a need for the opt-out functionality. It is always easier to add functionality than to remove it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks great. I've left a few comments that fixes current docs rendering issues, as well as a few nitpick comments mostly about naming (maybe it was just me at this hour trying to keep track of what is a Feature
instance, and what is a new pyvo feature, etc. but because of that it would be nice to use more distinct names).
Overall I approve it as such as I don't need to come back and review this again.
|
||
In order to activate a feature, users need to call the function:: | ||
|
||
activate_features('feature_one', 'feature_two') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It' probably OK for this module, but none of these code examples are picked up as doctests (but it's OK as it's pseudo code, if they should run as doctests then full executable examples are required)/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was intentional for the reasons you mentioned. The risk is for these examples to get out of sync with the implementation, but turning these snippet into doctests would make the documentation significantly more complex, and I'm not sure if it's worth it in this case.
try: | ||
prototype_function(probe) | ||
except Exception as exc: | ||
assert False, f"Should not have raised {exc}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this try/except?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just to make the intent more expressive. The function is supposed to throw an exception unless a condition is met, this might better express that when reviewing the code.
pyvo/utils/tests/test_prototype.py
Outdated
probe = mock.Mock() | ||
|
||
@prototype_feature('class') | ||
class FeatureClass: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nitpick: maybe use a more prototype like name. (It's a bit confusing that there is a Feature
class, but that is for the inner working, and then there are the pyvo prototype features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the name to SomePrototype
Thanks @bsipocz will do. |
Co-authored-by: Adrian <adrian.damian@nrc.ca>
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Just pushed a bunch of small cosmetic/docs changes addressing the feedback I got. Where the resolution was obvious I resolved the threads. Where I expect potential additional feedback I left the thread open. |
Thank you for doing this @olaurino. It looks ready to me after the test is fixed. |
oops, sorry! Long weekend! |
On Fri, Apr 29, 2022 at 05:45:59AM -0700, Omar Laurino wrote:
I am starting to think that having to opt-out is unsafe anyway.
Clients will have to know that even though it's unlikely, the API
might change, even ever so slightly. So even though they won't need
to opt-in, they would need to make sure they plan for a change of
API, which translates to a de-facto opt-in anyway.
Well... "safe"... Given we're talking about networked services here,
I'd guess in general other trouble (services temporarily down,
services going away, schemas changing) are a far greater danger to
scripts than post-WD API changes.
Be that as it may: I'd not argue against a suitably non-alarming
warning ("This is an experimental feature -- it *might* change
behaviour without prior warning").
There is an alternative I could also live with, but that's probably a
lot of work. That would be global opt-in, but the decorator would
(a) add a "This is an experimental feature. Say <appropriate code>
in your script to enable it" and (b) the error message when people
use it anyway gives similarly clear explanations on how to opt in.
But as I said: I suspect global opt-in is hedging too much.
…-- Markus
|
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
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 warnpyvo
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 warningpyvo to prevent me from inadvertently calling a prototype feature.As a pyvo client I want to be able to
completely turn offactivate one, several, or all experimental features, so the system does not errorsout 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 newprototype
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:
or
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.