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

Add option to remove event handler from luigi.Task #3282

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

starhel
Copy link
Contributor

@starhel starhel commented Mar 13, 2024

Description

I'd like to introduce luigi.Task.remove_event_handler method to support removing one of registered callbacks. By invoking this method with the appropriate parameters (event and callback), the specified event handler is removed from the internal registry.

Motivation and Context

I have many tests in which I need to collect all exceptions from a graph. Since luigi.build does not return a list of exceptions, I've discovered that adding an event handler is the simplest way to achieve this. However, there's currently no option to remove the handler in the teardown. As a workaround, I must maintain a hack with a private field of the Task class for now.

@pytest.fixture
def luigi_exceptions():
    capture = LuigiExceptionCapture()  # object to collect exceptions which works with multiprocessing

    @luigi.Task.event_handler(luigi.Event.FAILURE)
    def failure_handler(task, ext):
        capture.add(task, ext)

    yield capture

    luigi.Task._event_callbacks[luigi.Task][luigi.Event.FAILURE].remove(failure_handler)

Have you tested this? If so, how?

Above fixture is utilized in my projects, and I've also added a unittest. :)

@starhel
Copy link
Contributor Author

starhel commented Apr 9, 2024

Hi @dlstadther, just wanted to ask if you could spare a moment to review and merge this PR. If you need any additional information or have any questions about the code, please don't hesitate to reach out.

@dlstadther dlstadther merged commit ed530b8 into spotify:master Apr 14, 2024
42 checks passed
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 this pull request may close these issues.

2 participants