enthought / ets

ets.py is a utility to clone and manage various Enthought Tool Suite packages
http://docs.enthought.com/ets
Other
34 stars 11 forks source link

Pending/Internal deprecation warning? #37

Closed kitchoi closed 4 years ago

kitchoi commented 4 years ago

Summary from discussion on another channel for context: When Traits deprecates something and raises deprecation warnings, other ETS projects need to adjust. On the one hand, if there were no deprecation warnings, it is harder to motivate projects to adjust. On the other hand, further downstream projects depending on ETS may find these deprecation warnings annoying as those warnings are out of their control.

This is a general problem not specific to Traits. One can replace "Traits" with "TraitsUI", "other ETS projects" with "Mayavi". Or "Traits" with "Python", and "other ETS packages" with other popular libraries such as sqlalchemy. But given the close collaboration among ETS packages, more could be done to mitigate some of that warning noise while achieving the modernization goal.

I will try to avoid referring directly to Traits, but just "upstream ETS projects", "downstream ETS projects", and everyone else.

To resolve this, the deprecation warnings need to have staged audiences, not unlike the original intention of having PendingDeprecationWarning and DeprecationWarning (and many more). However in this discussion, it is evident that some believe the PendingDeprecationWarning is not useful, but some still think it is useful to keep. I think we can learn from this discussion.

For the purpose of discussion and to avoid confusion, let's say we can have two types of deprecation warnings: "InternalDeprecationWarning" (I made it up) and "DeprecationWarning" (the Python one). InternalDeprecationWarning is intended for ETS projects. DeprecationWarning is for everyone else.

Drawing lessons from the discussion thread referenced above, for this staged mechanism to work: (1) The "InternalDeprecationWarning" and "DeprecationWarning" should have different visibility. (2) There should be a way to remind developers to bump "InternalDeprecationWarning" to "DeprecationWarning" in the appropriate release versions (e.g. this helper function)

On (1), an ETS projects may suppress its own InternalDeprecationWarning unless an environment variable is set. All ETS projects can set this environment variable in their CI to make these warnings visible. (The one at the bottom of the stack probably does not have to). However one caveat is that this assumes one downstream ETS project is updated and released as frequently as another upstream ETS projects are at bumping these deprecation stages.

e.g. Project B depends on Project A. Project A may emit InternalDeprecationWarning at version 2.0, and the bump the warning to DeprecationWarning to 3.0. Throughout that time, Project B has not been maintained actively or it may not have released changes dealing with the deprecation. Then everyone else depending on ETS will see the annoying DeprecationWarning as if no efforts have been made to stage the warnings.

That said, maybe this is an experiment that can be tried?

Opened for discussion.

mdickinson commented 4 years ago

Suppose we add a new warning type of InternalDeprecationWarning. How do we go about preventing non-ETS projects from seeing InternalDeprecationWarnings in their test runs?

kitchoi commented 4 years ago

OK, so here is a draft implementation...

In the upstream project:

def emit_internal_deprecation_warning(msg, stack):
    if os.environ.get("ETS_INTERNAL", False):
        warnings.warn(msg, InternalDeprecationWarning, stack=stack)

In the downstream project, the CI setup will set this ETS_INTERNAL environment variable to a truthy value. Developers are encouraged to set this environment variable for their environment if they are not using the same command CI uses.

Maybe there are some cleverer ways.

mdickinson commented 4 years ago

Ah, okay; if we provide internal_warning, we don't really have a need for a separate warning type: we could simply use DeprecationWarning.

mdickinson commented 4 years ago

(Or if we really want to distinguish the types, use the PendingDeprecationWarning, just so that the warnings look different.)

kitchoi commented 4 years ago

Yep, using PendingDeprecationWarning would also be fine. I was just trying to avoid confusion when I invented one for the purpose of discussion.

mdickinson commented 4 years ago

I think something like this could work. Presumably you'd want to add emit_internal_deprecation_warning (modulo naming) to one of the Traits util modules, so that it's available to the rest of ETS? That's not ideal, but if we only plan to use it within ETS then we don't have to make it part of any of the api modules and officially support it for the wider world.

kitchoi commented 4 years ago

Yep. Sounds reasonable to me!

kitchoi commented 4 years ago

Offline discussion: Not going to proceed. We will diligently track usage within ETS (e.g. search code with regex) when something is being deprecated, open issues for those usages in the corresponding repositories, and then fix those using the issues as guide. Should an upstream ETS project be released with new deprecation warnings and another ETS project has not fixed or released its fix, it will impact ETS-using project developers, but most of the time it can be resolved by installing warning filters.

Closing. (Again, any issue can always be reopened for reconsideration.)