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 issue with uptodate calls #5

Closed
wants to merge 1 commit into from
Closed

Conversation

pveglia
Copy link

@pveglia pveglia commented Sep 30, 2014

Up-to-date checking used to short circuit calls, so if first item
in the uptodate list returned False, subsequent items were not
called. This introduced weird behaviors when using checkers with
side effect such as result_dep; in that case, outcome depended
on items order.

This patch removes the "short-circuiting" logic and forces every
up-to-date checker to be called.

Up-to-date checking used to short circuit calls, so if first item
in the `uptodate` list returned `False`, subsequent items were not
called. This introduced weird behaviors when using checkers with
side effect such as `result_dep`; in that case, outcome depended
on items order.

This patch removes the "short-circuiting" logic and forces every
up-to-date checker to be called.
@schettino72
Copy link
Member

"short-circuiting" is the intended behaviour. The reasoning is to avoid performing unnecessary checks.

Having side-effects on checkers is bad idea. Can you give a concrete example where this is desirable? Sorry, but unless there is a strong use-case for this it will be rejected.

The docs doesnt mention the "short-circuit" behaviour. So the docs need to be improved.

@pveglia
Copy link
Author

pveglia commented Sep 30, 2014

Hi Eduardo,

thank you for your quick reply!

A concrete example is the result_dep function in doit/tools module. When
result_dep is the second item in the uptodate list and first element
evaluates to False, its __call__ method is never called and action result
cannot be stored, resulting thus in a wrong computation of task
uptodate-ness.

Another concrete case could be the following: one of my needs is to access
values stored by previous invocations from action. For the time being I use
an uptodate checker which does (among other things):

def checker(task, values):
task.oldvalues = values

So that in my action i can access the oldvalues field in task. Maybe
there's a better way to do that but I could not find how.

I agree with you that short-circuiting avoids unnecessary work but, at the
moment, since result_dep has side effects, all uptodate functions sould
be evaluated.

If I'll come up with a better solution to solve this problem, preferably
avoiding side-effects in uptodate checking I'll let you know.

Thank you for your time and for your tool, I like it very much!

On Tue, Sep 30, 2014 at 11:01 AM, eduardo naufel schettino <
notifications@github.com> wrote:

"short-circuiting" is the intended behaviour. The reasoning is to avoid
performing unnecessary checks.

Having side-effects on checkers is bad idea. Can you give a concrete
example where this is desirable? Sorry, but unless there is a strong
use-case for this it will be rejected.

The docs doesnt mention the "short-circuit" behaviour. So the docs need to
be improved.


Reply to this email directly or view it on GitHub
#5 (comment).

@schettino72
Copy link
Member

I guess I understand your motivation. But this is the wrong fix.

If you provide a patch where a task can access its old values inside the actions I will probably accept it :)
I am not sure how it would look like, maybe based on getargs. http://pydoit.org/dependencies.html#getargs

smarie pushed a commit to smarie/doit that referenced this pull request Jul 26, 2019
…or the main Task. A step towards pydoit#5 but still misses unit tests and validation of what should actually be done.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants