Skip to content
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

Provisional Warnings - Addressing the Pace of Developement #281

Closed
rmorshea opened this issue Aug 21, 2016 · 14 comments
Closed

Provisional Warnings - Addressing the Pace of Developement #281

rmorshea opened this issue Aug 21, 2016 · 14 comments

Comments

@rmorshea
Copy link
Contributor

With the pace of traitlets development in recent months, there's been a lot of concern over ease of adoption, and the ability of those who already have, to keep up with the each new release. To reduce this unease, It's been suggested that we implement "provisional warnings" which will raise an exception explaining the instability of various features by default, but that can be manually silenced with a context manager.

The purpose of these warnings would be to create a space where traitlets can develop at a natural rate (fast or slow), but do so behind bold warning tape. A provisional warning will explain to any developer so bold as to bypass it, that the feature they are accessing is not guaranteed to exist over any period of time in the future, whether that be in the same form, or at all.

I hope to determine:

  1. Whether provisional warnings are reasonable
  2. What features ought to have provisional warnings
  3. How provisional warnings should be implemented.
@minrk
Copy link
Member

minrk commented Aug 26, 2016

What are some examples of proposals that would be provisional? Have there been any things introduced so far that would have gotten this treatment?

@rmorshea
Copy link
Contributor Author

rmorshea commented Aug 26, 2016

#230, #242, #261, #278, could be provisional until we're comfortable with their stability. As for past installments, the new decorator API would have been a great feature to release provisionally.

Overall though, I don't really see any downside to releasing most new features as provisional for a period of time. Since we have such a small group developing traitlets right now, it will be much easier to tease out unforeseen bugs if we use willing early adopters as leverage against them.

I envision this kind of warning as a way to safely merge unstable features into master so they get some testing before we remove the caution tape. To make this work though, it would be nice if there was a way to advertise that these provisional additions exist so people will be more likely to go out of their way to try them.

@minrk
Copy link
Member

minrk commented Aug 30, 2016

How would automatic documentation be made provisional? What would the API look like to load and execute provisional auto-gen-docs? I see the use case for new trait types, but I'm not quite sure how it would look to implement / use the provisional things that have deeper levels of integration.

@Carreau
Copy link
Member

Carreau commented Aug 30, 2016

I think we can have a decorator for functions (@provisional) that make functions (like @undoc does and maybe "just" a _provisional=True.

It's also mostly a question on marking things explicitly for user "Hey this is going to change we don't commit to an API, so we can also make it explicit in the docstring.

If the Provisional warnings raise by default building the doc is a matter of injecting a filter in warnings in conf.py as far as I understand.

One thing I'm still thinking about is wether Provisional things should log or raise by default.

  • If they raise, we are sure that only advance user use it (with a context manager), but we will have few usage.
  • If they print by default, we can have wider usage and it's easy (context manager again) to silence locally.

Also if we make something provisional we likely don't want to integrate it too deeply.

@rmorshea
Copy link
Contributor Author

rmorshea commented Aug 30, 2016

@minrk, I think we'd have to be more sure of our plans to implement auto-docstrings before I could really say if and/or how the feature could be made provisional with a context manager. However your concern over more deeply embedded features does stand, and makes the idea of a context manager seem a bit unwieldy.

@Carreau
Copy link
Member

Carreau commented Aug 30, 2016

However your concern over more deeply embedded features does stand, and makes the idea of a context manager seem a bit unwieldy.

I don't see why it's that hard. You can make a filter that disable all ProvisionalWarnings for traitlets at once – no need to list all of them – if you are using a Provisional Feature whether or not it uses other one is an implementation detail. It's basically like when you sign a waiver that you know you take all responsibility, or you ignore a sign.

@rmorshea
Copy link
Contributor Author

rmorshea commented Aug 30, 2016

Given the interdependance within traitlets, I can imagine cases where the safe one-time activation of a provisional feature would inadvertently toggle the outcome of a hidden condition and raise unexpected warnings later:

class A(HasTraits):
    condition = False

    def unsafe(self):
        warn("provisional feature")
        self.condition = True

    def safe_if(self):
        if not self.condition:
            # do something safe
        else:
            warn("provisional feature")
            # do something unsafe

Instead of using the manager for small code snippets, this could encourage people to write their whole test script in the silenced context to avoid chained warnings.

Perhaps a better option would be to use sys.meta_path to engage filters and post a warning:

from traitlets import provisional_features

provisional_features could then be a simple object holding docstrings for provisional features that could be inspected with previsional_features? or previsional_features.name?

@Carreau
Copy link
Member

Carreau commented Aug 30, 2016

Given the interdependence within traitlets, I can imagine cases where the safe one-time activation of a provisional feature would inadvertently toggle the outcome of a hidden condition and raise unexpected warnings later:

It seem like safe_if is unsafe. If you have a way to access code that might change behavior later, then by nature it is unsafe, and that's the all point of the ProvisionalWarnings:

"Hey you might not have realized it but this thing might change API don't expect it to work later"

And I can make the arguments for provisional import as well:

try:
   import provisional
except:
  provisional = None

class A(HasTraits):
    condition = False

    def safe_if(self):
        if not provisioanal:
            # do stuff
        else:
            provisional.do_fancy_stuff()

The point of provisional is to make a usable API that show warnings that can be turned into a strict "Should not touch", or "mute and deal with consequences" at will.

Assuming safe_if is supposed to be "stable" then to be "save" it should be:

def safe_if(self):
        if not self.condition:
            # do something safe
        else:
            with mute_provisional()"
                # do something unsafe
                # I, as a programmer using mute_provisional engage
                # into not complaining if APi change or is removed. 

Alternatively you can have a

def stable_if(callback):
        # I want to make sure provisional
        # feature are not used there. 
        with forbid_provisional()"
            callback()

Having the provisional Warnings is not incompatible with also scoping in a provisional module,
though scoping in a provisional module ensure we will break api when we rename while a provisional functionality may loose it's warnings without changes in which case all the existing warning muting/enforcing guards just become no-op.

@rmorshea
Copy link
Contributor Author

rmorshea commented Aug 30, 2016

@Carreau, I may have summarized my meta_path idea too much. I didn't mean to imply that provisional features would actually be imported from a separate module called provisional_features. Instead it would look something like this:

# in traitlets/traitlets.py

class provisional_features(object):
    # a class that holds docstrings for provisional features
    my_feature = "explain it"

class ProvisionalFilterImporter(object):
    def find_module(self, fullname, path):
        if fullname.split('.')[-2:] == ['traitlets', 'provisional_features']:
            # engage warning filters

sys.meta_path.insert(0, ProvisionalFilterImporter())

This way import traitlets.provisional_features would engage a warning filter for the file through the ProvisionalFilterImporter which was appended to sys.meta_path, but actually return the class provisional_features.

(normally I would just make a pr, but I only have my phone right now)

@rmorshea
Copy link
Contributor Author

rmorshea commented Aug 30, 2016

As for my concern with the context manager, I don't think safe_if should be treated like it's safe. I agree that it is unsafe. Instead, my worry is that people will write all of their experimental scripts inside the context because of how these unsafe features will chain together:

import traitlets
with traitlets.muted_provisional_warnings():
    # ... all the rest of the script ...

Which wouldn't really be much different from the import idea I clarified above.

@Carreau
Copy link
Member

Carreau commented Aug 30, 2016

I agree that it is unsafe. Instead, my worry is that people will write all of their experimental scripts inside the context because of how these unsafe features will chain together:

Well they wouldn't even ned the context manager, they would just show a mute warning in the warning filter; but the point is then that they did that on purpose, so the blame is on them.

Moreover the context manager at script level wouldn't work as it will be triggered at import time/ function definition time and would still raise at call time.

@minrk
Copy link
Member

minrk commented Aug 31, 2016

But for something like autodoc, we would use it in traitlets.config, and thus it would be inherited by all Configurable classes in the wild. I don't see how that could be made a user choice.

@rmorshea
Copy link
Contributor Author

rmorshea commented Sep 2, 2016

I'll document provisionals more soon, however if something like this existed as a seperate package the following could be possible.

# user's experiment
import provisional_features as prov
# global allowance
prov.allow("my_feature")
import traitlets
# in traitlets.py
import provisional_features as prov
if "my_feature" in prov.allowed_features():
    # enable feature without fear of warning
else:
    # don't enable the feature

@minrk, however this would be an extremely niche case, and if we have to resort to creating a new package to satisfy it, I don't think it's worth making autodocs provisional.

I've also added filter priority:

# introduce the idea of filter priority
with prov.allowed_context(other_feature=2):
    with prov.blocked_context(other_feature=1):
        # in this context "other_feature" will warn
    # in this context "other_feature" won't warn

@rmorshea
Copy link
Contributor Author

I don't think this is going to happen. Feel free to reopen if it's a desirable feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants