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

Deduping broken? #123

Closed
bitprophet opened this issue Feb 28, 2014 · 5 comments
Closed

Deduping broken? #123

bitprophet opened this issue Feb 28, 2014 · 5 comments

Comments

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Feb 28, 2014

Report from the ML via @flamingbear

Operating System = Mac OS X 10.9.1

(invoke)savoie@savoie-laptop ~/projects/invoke-test$ python --version
Python 2.7.6

(invoke)savoie@savoie-laptop ~/projects/invoke-test$ pip freeze
-e git+https://github.com/flamingbear/invoke.git@f6f7fca00be719841d8d8da6f2ce82e3f66123b7#egg=invoke-master
wsgiref==0.1.2

tasks.py: https://gist.github.com/flamingbear/9272702

from invoke import task, run

@task
def clean():
    print("Cleaning")

@task('clean')
def build():
    print("Building")

@task(pre=['build'])
def package():
    print("Packaging")

and the output: https://gist.github.com/flamingbear/9272722

(invoke)savoie@savoie-laptop ~/projects/invoke-test$ invoke build package
Cleaning
Building
Building
Packaging
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Feb 28, 2014

I checked real quick and the docs + state of Executor both imply deduping should be on by default, and there's basic tests proving it, so this is likely a case or cases not tested for, eg:

  • 2x tasks called at one time; I think my existing test covers this but it's low level so there may be interplay between CLI and Executor not tested;
  • One of the tasks is both explicitly given and is specified as a pre-task of the other explicitly given task.
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented May 27, 2014

Back on this and #120 after fixing #135 \o/

Double checked and the error in this ticket is because currently execution considers each top level (CLI-given) task independently, re: pre/post tasks and deduping.

I feel like it would probably make sense for them to "know about" each other for deduplication purposes.

I'm torn on whether we'd want this to be the case when they are not consecutive, e.g. two tasks both calling a clean beforehand might want to be clean -> taskA -> clean -> taskB instead of clean -> taskA -> taskB. Will leave duplication in its existing 'aggressive' mode for now; we can probably add a third 'consecutive only' option later if people seem to need it.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented May 27, 2014

Now wondering exactly why I stuck in "call tracking" based deduping too, at some point (apparently in 511393d). Or rather, why it and the other form of deduping (the one that looks at the entire list and tries removing duplicates before executing) are both implemented - they overlap significantly in behavior. IIRC it was done as a quick-and-easy method of deduplicating before the recent shakeup.

The downside of call tracking (which uses state within Task instances) is that it won't play nicely with multiprocess/threaded/etc forms of parallelism, which are planned. However, the "check before you start" methodology still would.

Feels like leaving the call tracking implementation in (because it's useful metadata) but not using it in deduplication is the way to go for now.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented May 28, 2014

Seem to have this working now. Left to do:

  • Finish remaining TODOs in the newly reworked Executor code
  • Make sure some more tests get written or skipped tests get filled out (eg at least some TODOs should spawn new tests)
  • Implement post-tasks now while we're at it, should be trivial (famous last words)
  • Write/update a changelog entry spanning this, #120, etc (feel like there was a third one in there?)

Also noting for the record/linkage that #45 still exists but I do think at this point, that level of extra control can and should wait. Probably wants the newer execute() to be teased out into more subroutines for extensibility - then a subclass implementing eg "dedupe pre/post tasks overlapping CLI ones, but not amongs themselves" would be a real possibility.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 9, 2014

All done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.