conda / ceps

Conda Enhancement Proposals
Creative Commons Zero v1.0 Universal
20 stars 24 forks source link

Conda Generic Plugin Hooks #45

Open travishathaway opened 1 year ago

travishathaway commented 1 year ago

This is a CEP for implementing a set of generic plugin hooks for conda. Included so far are the following hooks:

☝️ This list is not final, and we welcome a healthy discussion to debate exactly what belongs here.

kenodegard commented 1 year ago

would it be better to reuse existing terminology (e.g., *_subcommand)?

travishathaway commented 1 year ago

would it be better to reuse existing terminology (e.g., *_subcommand)?

@kenodegard , I'm not exactly sure what that means. Can you please clarify a little?

kenodegard commented 1 year ago

@travishathaway as in use pre_subcommand/post_subcommand instead. It isn't clear what "run" means in pre_run/post_run. Where does it occur? Is it only applicable for the conda run command? Etc.

travishathaway commented 1 year ago

@travishathaway as in use pre_subcommand/post_subcommand instead. It isn't clear what "run" means in pre_run/post_run. Where does it occur? Is it only applicable for the conda run command? Etc.

@kenodegard , I think I'm going to just go with pre_command and post_command to make it just a tad bit shorter to type. It conveys the same meaning.

travishathaway commented 1 year ago

@jezdez, thanks for the review.

When I created the on_exception handler, I was thinking of it more as pseudo event handling system. But, it sounds like what you think may be better is to just allow people to replace or subclass the conda.exceptions.ExceptionHandler class instead.

The "exception_handler" hook would then function just like the "solver" hook. You would only be allowed to configure a single exception handler.

I'm thinking this would be easier from a plugin author's perspective. All they would have to do is the following:

from conda.exceptions import ExceptionHandler

# Example for implementing pretty printing exceptions with "rich"
from rich.console import Console
console = Console()

PLUGN_NAME = "rich"

class RichExceptionHandler(ExceptionHandler):
    """
    Custom exception handler that uses rich to print exceptions.
    """

    def handle_exception(self, exc_val, exc_tb):
        console.print_exception(show_locals=True)

@hookimpl
def conda_exception_handlers(exception):
    """
    Returns our CondaExceptionHandler class which attaches our ``print_exception(``.
    """
    yield CondaExceptionHandler(
        name=f"{PLUGIN_NAME}_exception_handler",
        exception_handler=RichExceptionHandler
    )

We could change the function signature of the handle_exception method on the ExceptionHandler class to pass in the actual exception object too.

What do you think? Is relying on inheritance here a Good Idea™️?

jezdez commented 1 year ago

@travishathaway Okay, thinking about this a little more, I think we'll have to handle a situation in which multiple plugins implement exception handlers and exist in the runtime environment at the same time.

E.g. imagine if conda-libmamba-solver shipped an exception handler that would enable it to print out additional details of solver errors (e.g. https://github.com/conda/conda-libmamba-solver/issues/102) and the user in addition would like to install a handler that generally renders exceptions with rich (like your example above). The "solver" hook works in that way: there can be multiple solvers installed, and the default setting and the config system determine which to use. In case of exceptions handlers, we probably shouldn't require users to enable them before using them, though. Or is that an indicator to require a plugins enable and plugins disable command to make this an explicit process?

My question is whether we want to enable multiple exception handlers to coexist and create a kind of exception handling pipeline instead of enabling exactly one exception handler?

exception_handlers = plugin_manager.get_hook_results("exception_handlers")
for exception_handler in exception_handlers:
    result = exception_handler.handler(exc_val, exc_tb):
    if result is not None
        return result

The API could require exception handlers to return if they've handled all exceptions, e.g.

class LibmambaExceptionHandler(ExceptionHandler):
    """
    When encountering a libmamba specific error, print it nicely.
    """
    def handle_exception(self, exc_val, exc_tb):
        if isinstance(exc_val, LibMambaUnsatisfiableError):
            return self.handle_libmamba_unsatisfiable_exception(exc_val, exc_tb)

    def handle_libmamba_unsatisfiable_exception(self, exception, traceback):
        ...

@hookimpl
def conda_exception_handlers(exception):
    """
    Returns our CondaExceptionHandler class specific to libmamba exceptions.
    """
    yield CondaExceptionHandler(
        name=f"{PLUGIN_NAME}_exception_handler",
        exception_handler=LibmambaExceptionHandler
    )

@jaimergp You've looked at https://github.com/conda/conda-libmamba-solver/issues/102 a bunch and I think @wolfv has thought about that as well, what's your take on this topic?

jaimergp commented 1 year ago

A bunch of exceptions in conda are error messages targeting the CLI output for the end user. There's little programmatic intent, and we've often had no other option than to parse error messages back and forth to obtain the required metadata to intercept the exception and handle it.

We could argue that exceptions should be more library friendly, and not so focused on the message but... I actually think it could be seen as a symptom of a bigger problem: exceptions in conda and friends are often used as flow control mechanisms, as an impromptu replacement for more adequate APIs. "I can't solve this? Let's thrown an exception and we'll hope it's caught in the right place!".

Some examples of exceptions-as-flow-control (anti?)pattern:

So maybe that little mess is something to discuss before or while designing a custom exception handler mechanism? I do agree that having multiple exceptions handlers coexisting seems less limiting than a chain of subclasses. Didn't Python add support for Exception groups or something like that? Is that useful in this case?

travishathaway commented 1 year ago

@jezdez @aovasylenko @jaimergp

I have thought about this some and have decided to take @jaimergp's advice to not include a custom exception handler yet. To use a metaphor, I think we need to clean up the house a bit before inviting guests in 😅. I think that a refactor of the current exception handling mechanism should be done before we revisit this. Once that has been done, we can restart this conversation with a separate CEP.

In the meantime, I will remove exception handling from this CEP and just stick with the pre_command and post_command hooks. These already enable quite a bit.

Once I've removed the exception handling pieces from this CEP, I will move it out of "draft" status. Hopefully, the vote will follow soon afterwards 👍.

travishathaway commented 1 year ago

Example of how our plugin developers could use these hooks:

XuehaiPan commented 1 year ago

Hi, some questions about the hooks. As I post in the description in PR conda/conda#10567:

A core usage is to define environment variables when activating/deactivating a conda environment. E.g.:

export CMAKE_PREFIX_PATH="${CONDA_PREFIX}"

NOTE: conda env config vars set does not support variable substitution. So we need hooks here.

In conda/conda#10567, I propose to create a post-create hook to add env-vars.sh to CONDA_PREFIX/etc/activate.d and CONDA_PREFIX/etc/deactivate.d on each environment creation. That approach is covered by this CEP. Now, I can create a CondaPostCommand with run_for={"create"}.

I wonder can the hooks affect the current shell environment like CONDA_PREFIX/etc/activate.d/*.sh do? (not the process of the conda command, which is a subprocess) So I can write post-activate hooks directly rather than etc/activate.d/*.sh. It would be nice to have an example of this in the documentation too.

# post-activate hook
export CONDA_CMAKE_PREFIX_PATH_BACKUP="${CMAKE_PREFIX_PATH}"
export CONDA_LD_LIBRARY_PATH_BACKUP="${LD_LIBRARY_PATH}"
export CMAKE_PREFIX_PATH="${CONDA_PREFIX}"
export LD_LIBRARY_PATH="${CONDA_PREFIX}/lib:${LD_LIBRARY_PATH}"
# pre-deactivate hook
export CMAKE_PREFIX_PATH="${CONDA_CMAKE_PREFIX_PATH_BACKUP}"
export LD_LIBRARY_PATH="${CONDA_LD_LIBRARY_PATH_BACKUP}"
unset CONDA_CMAKE_PREFIX_PATH_BACKUP
unset CONDA_LD_LIBRARY_PATH_BACKUP
XuehaiPan commented 1 year ago

We may also need to document the execution order when multiple hooks are run for the same event:

hook1: run_for={"install", "create"}
hook2: run_for={"install", "update"}

for example, lexicographic order, like ~/.ipython/profile_default/startup/README:

This is the IPython startup directory

.py and .ipy files in this directory will be run *prior* to any code or files specified
via the exec_lines or exec_files configurables whenever you load this profile.

Files will be run in lexicographical order, so you can control the execution order of files
with a prefix, e.g.::

    00-first.py
    50-middle.py
    99-last.ipy

We need to document the order of hook1.pre_command vs. hook2.pre_command and hook1.post_command vs. hook2.post_command. Maybe a FILO stack order?

   hook1.pre_command
-> hook2.pre_command
-> conda command
-> hook2.post_command
-> hook1.post_command
travishathaway commented 1 year ago

@XuehaiPan,

Thanks for your feedback. To answer your question, no this not possible. I researched this myself once and came to the conclusion that simply is not possible because of how shells work (sub-process cannot modify the parent process' environment variable values). This is why we use so much shell specific code in conda and why it's necessary for us to modify a user's shell configuration file (e.g. .zshrc and .bashrc).

jaimergp commented 1 year ago

@XuehaiPan - I think such a plugin would need to add the relevant instructions in (de)activate.d/ directories (or conda-meta/state) as part of its installation or command-line effects. Not that running arbitrary code is a good idea in general, but I get that it's somehow an established practice in the conda ecosystem.