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

fix: Independent decorator task exec lifecycles #2231

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

saikonen
Copy link
Collaborator

Exceptions during task exec lifecycles (task_pre_step/task_decorate/task_post_step) in decorators can currently impact the calling of other decorator lifecycles.

Example:

from metaflow import step, FlowSpec, secrets, kubernetes

class BreakSecretFlow(FlowSpec):

    @kubernetes
    @secrets(sources=["test-secret-with-no-permissions"])
    @step
    def start(self):
        print("Testing secrets..")
        self.next(self.end)

    @step
    def end(self):
        print("Done! 🏁")


if __name__ == "__main__":
    BreakSecretFlow()

The above flow will currently break due to

  1. @secrets raising an exception
  2. which prevents @kubernetes from having task_pre_step called, which is supposed to bind self.metadata
  3. The missing self.metadata in turn makes task_finished fail for @kubernetes.

Issues:

  • the true error is masked under @kubernetes exceptions, making debugging nontrivial
  • due to task_pre_step being blocked for @kubernetes, we end up missing metadata records (pod name etc.) which would have been recorded during this.

Proposal is to instead execute all the lifecycle methods independently, collecting exceptions and raising only at the end of iteration. This will ensure that decorators do not interfere with each others lifecycles via exceptions, and a decorator can more reliably perform data passing between its own lifecycle methods

Side note: The issue is dependent on decorator ordering.

# This will break on the kubernetes decorator.
@kubernetes
@secrets(sources=["test-secret-with-no-permissions"])

# This will instead surface the secrets decorator exception.
@secrets(sources=["test-secret-with-no-permissions"])
@kubernetes

@saikonen saikonen requested a review from romain-intel January 29, 2025 12:35
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.

1 participant