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

Change Conditions Execution Order #701

Open
b-butler opened this issue Dec 9, 2022 · 1 comment
Open

Change Conditions Execution Order #701

b-butler opened this issue Dec 9, 2022 · 1 comment
Labels
good first issue Good for newcomers

Comments

@b-butler
Copy link
Member

b-butler commented Dec 9, 2022

Description

Currently we execute operation decorators from outside in or top bottom. This is confusing when using decorators as functions directly.

@FlowProject.operation
def op(job):
    pass


FlowProject.pre(expesive_cond)(FlowProject.pre.true("foo")(op))

This actually runs the expensive computation first. This is to make this

@FlowProject.pre.true("foo")
@FlowProject.pre(expensive_cond)
@FlowProject.operation
def op(job):
    pass

more intuitive. However, I disagree that this is more intuitive. As someone learns Python in fact this begins to become less and less intuitive to the point that without our documentation suggesting the correct ordering, a Python expert would write,

@FlowProject.pre(expensive_cond)
@FlowProject.pre.true("foo")
@FlowProject.operation
def op(job):
    pass

Given our recent decorator ordering requirements we are already making users come to understand decorators apply bottom to top.

Suggestion

I think we should apply conditions in the order they come in the execution (i.e. bottom to top). As an irrelevant side note this would make the project definition run faster.

@vyasr
Copy link
Contributor

vyasr commented Dec 13, 2022

I don't have a strong opinion on which visual ordering is more intuitive to users, I'll let others chime in on that discussion. I do agree that matching the decorator ordering to the behavior of the imperative API makes sense, and that the recent changes are forcing more explicit discussion of the implications of decorator evaluation and ordering anyway, so I'm not opposed to this change.

AFAIR we document the condition ordering in only one place, and I don't know how many people are really aware of this, so I don't anticipate this being a big surprise to users if there is support for it.

@b-butler b-butler added the good first issue Good for newcomers label Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants