Improve RoundEngine lifecycle.

Review Request #1665 — Created Jan. 25, 2015 and submitted

jsirois
pants
jsirois/engine/lifecycle_improvements
992
6a0636e...
pants-reviews
benjyw, davidt, fkorotkov, ity, patricklaw

This change lifts all engine planning methods needed to prepare the task
graph for a run to be task classmethods. In addtion it introduces a new
alternate_target_roots lifecycle method to support more disciplined
replacement of the active target set. As part of this move,
Context.replace_targets is made ~private to discourage its direct use.

The task graph preparation lifecycle is now the following sequence of
task classmethods:
1. register_options - declare options configurable via cmd-line flag
or config file.
2. product_types - declare the product types your task is capable of
producing.
3. alternate_target_roots - propose a different set of target roots to
use than those specified via the CLI for the active pants run.
4. prepare - request any products needed from other tasks.

The task execution lifecycle is now just construct and then execute,
back to back.

Note that this fix is just a temporary aid in the transition to the tuple
engine.

Fixes along the way:
+ Fixup all Tasks to use the new lifecycle.
+ Fixup tests using Context.replace_targets when they need not have been.
+ Re-work ChangedFileTaskMixin for use with alternate_target_roots in the changed goals.
+ Kill broken and unused BuildLint task.

Several more fixes pointed at by broken tests:
+ Have Depmap use prepare to create a conditional dependency on resolve
and cleanup DepmapIntegrationTest.
+ Fix ProtobufIntegrationTest.test_source_ordering to ensure its
assumption of a fresh gen, also fixup tests for "output contains".
+ Fix WireIntegrationTest.test_good to ensure its assumption of a fresh
gen, also fixup tests for "output contains".

src/python/pants/backend/android/tasks/aapt_builder.py | 11 ++--
src/python/pants/backend/android/tasks/aapt_gen.py | 20 ++++----
src/python/pants/backend/android/tasks/dx_compile.py | 21 ++++----
src/python/pants/backend/android/tasks/jarsigner_task.py | 11 ++--
src/python/pants/backend/codegen/tasks/code_gen.py | 7 +--
src/python/pants/backend/codegen/tasks/protobuf_gen.py | 21 +++++---
src/python/pants/backend/codegen/tasks/thrift_linter.py | 15 +++---
src/python/pants/backend/core/register.py | 3 --
src/python/pants/backend/core/tasks/BUILD | 12 -----
src/python/pants/backend/core/tasks/build_lint.py | 116 ------------------------------------------
src/python/pants/backend/core/tasks/changed_target_goals.py | 28 +++++++----
src/python/pants/backend/core/tasks/confluence_publish.py | 7 +--
src/python/pants/backend/core/tasks/deferred_sources_mapper.py | 11 ++--
src/python/pants/backend/core/tasks/group_task.py | 26 ++++++----
src/python/pants/backend/core/tasks/prepare_resources.py | 14 +++---
src/python/pants/backend/core/tasks/task.py | 76 ++++++++++++++++++++--------
src/python/pants/backend/core/tasks/what_changed.py | 137 +++++++++++++++++++++++++++++++++-----------------
src/python/pants/backend/jvm/tasks/benchmark_run.py | 18 +++----
src/python/pants/backend/jvm/tasks/checkstyle.py | 11 ++--
src/python/pants/backend/jvm/tasks/depmap.py | 9 ++--
src/python/pants/backend/jvm/tasks/detect_duplicates.py | 9 ++--
src/python/pants/backend/jvm/tasks/ide_gen.py | 43 ++++++++--------
src/python/pants/backend/jvm/tasks/ivy_imports.py | 9 ++--
src/python/pants/backend/jvm/tasks/ivy_resolve.py | 28 +++++------
src/python/pants/backend/jvm/tasks/jar_publish.py | 11 ++--
src/python/pants/backend/jvm/tasks/jar_task.py | 9 ++--
src/python/pants/backend/jvm/tasks/junit_run.py | 17 ++++---
src/python/pants/backend/jvm/tasks/jvm_binary_task.py | 9 ++--
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py | 38 +++++++-------
src/python/pants/backend/jvm/tasks/jvm_run.py | 15 +++---
src/python/pants/backend/jvm/tasks/jvm_task.py | 7 +--
src/python/pants/backend/jvm/tasks/jvmdoc_gen.py | 15 +++---
src/python/pants/backend/jvm/tasks/provides.py | 9 ++--
src/python/pants/backend/jvm/tasks/scala_repl.py | 3 +-
src/python/pants/backend/jvm/tasks/specs_run.py | 17 ++++---
src/python/pants/backend/jvm/tasks/unpack_jars.py | 13 +++--
src/python/pants/base/lazy_source_mapper.py | 17 ++++---
src/python/pants/engine/round_engine.py | 70 ++++++++++++++++++--------
src/python/pants/goal/BUILD | 1 -
src/python/pants/goal/context.py | 28 ++++-------
src/python/pants/goal/error.py | 4 --
src/python/pants/goal/workspace.py | 7 ++-
src/python/pants/option/options.py | 5 ++
tests/python/pants_test/backend/jvm/tasks/test_checkstyle.py | 63 ++++++++++++-----------
tests/python/pants_test/backend/jvm/tasks/test_scalastyle.py | 72 ++++++++++++++------------
tests/python/pants_test/backend/jvm/tasks/test_unpack_jars.py | 2 +-
tests/python/pants_test/base/context_utils.py | 2 +
tests/python/pants_test/base/test_lazy_source_mapper.py | 9 +---
tests/python/pants_test/base_test.py | 4 +-
tests/python/pants_test/engine/test_round_engine.py | 51 ++++++++++++++-----
tests/python/pants_test/tasks/BUILD | 1 -
tests/python/pants_test/tasks/test_context.py | 22 ++------
tests/python/pants_test/tasks/test_depmap_integration.py | 31 +++---------
tests/python/pants_test/tasks/test_group_task.py | 40 ++++++++-------
tests/python/pants_test/tasks/test_jvm_run.py | 2 +-
tests/python/pants_test/tasks/test_protobuf_integration.py | 9 ++--
tests/python/pants_test/tasks/test_what_changed.py | 8 +--
tests/python/pants_test/tasks/test_wire_integration.py | 14 ++++--
58 files changed, 652 insertions(+), 636 deletions(-)

CI went green here: https://travis-ci.org/pantsbuild/pants/builds/48291884

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
JS
  1. Note: this is the bit of short-term restructuring that evolved from discussions with David Taylor in the post-summit engine discussion session.

  2. 
      
JS
  1. Another note: It's a small change (I have a patch ready if needed), to move Task construction back into the planning phase if folks have Tasks out there still that rely on the old construction in upstream order, execution downstream order semantics. It would be very nice though to not do this / allow this.

  2. 
      
BE
  1. Looks good! One question though: don't we call register_options() before product_types()?

    1. Yes - my head is still back in an incremental register model during the planning phase and in-fact options are registered globally across all known tasks for every run instead currently. Fixed TaskBase docs and this RB's description.

  2. It might be good hygiene to always call super's prepare(), even when it's not strictly necessary at present. Ditto for the prepare() classmethods in all other tasks.

    Although if this classmethod will go away entirely with the new engine then maybe it's not worth the trouble right now. Your call.

    1. I did a sweep for all non-direct Task descendants and fixed just those up.

  3. Presumably this comma is superfluous?

  4. G

JS
JS
  1. 
      
  2. Oops - did not mean to unregister / kill the BUILD for Provides - fix coming to revert this change to src/python/pants/backend/jvm/register.py and src/python/pants/backend/jvm/tasks/BUILD

  3. 
      
JS
BE
  1. Ship It!
    1. Thanks for taking a look Benjy. I'm going to hold off until David gets a chance to verify this satisfies his short term needs.

  2. 
      
JS
  1. Fedor - I have you on here in particular for a look at the Depmap / DepmapIntegrationTest changes. The 'ivy_jar_products' product is now conditionally required in the task graph preparation stage which means that resolve is added to the execution plan implicitly exactly when its needed, so there is no longer the awkward need to know to call ./pants resolve depmap --project-info and just ./pants depmap --project-info suffices.

  2. 
      
DA
  1. I'm very excited to see this changeset: I'd made a few false-starts on a similar refactor over the last month or so, but never managed to get it close to working.

    I jope that the strong separation between scheduling and task execution / mutable task state that we get by making the former into a classmethod will yield both short and long term benefits. The alternate-root lifecycle method is much cleaner than the previous assertion I was using, but more broadly, I expect the clearer demarcation of where tasks are expressing dependencies and where they are setting up or making assumptions about mutable state or context will make it easier, down the road, to identify code that can be (re)used in a next-gen scheduling world.

    Thanks!

  2. 
      
JS
  1. Thanks David - submitted @ https://github.com/pantsbuild/pants/commit/21346944b695e0b14c3154a5af5a15740c5b3e95

    Other folks, I'll be happy to address any concerns in a follow-up RB, in particular moving task construction to the task graph preparation stage iff folks have custom tasks that still rely on "construct in reverse execution order" semantics with no other way to solve things.

  2. 
      
JS
Review request changed

Status: Closed (submitted)

IT
  1. this is awesome - thank you for cleaning this up!

  2. 
      
PA
  1. Sorry for being super late to the game on this one. I'm very excited for this change, and it was well executed. I have a couple of comments and questions, but nothing major enough to warrant raising a post-submit issue.

  2. Why did we leave this as a (class)method that actually plumbs the round_manager? I was under the impression that it would be a classmethod that takes the options and yields up a list of products.

    1. As you figured out below - there are two types of products today and the odler non-data product is tricky with the predicate. This was the minimal use classmethods / add alternate_target_roots lifecycle method change.

  3. It's going to be very important that we plumb through spec_excludes and whatever whitelist/blacklist system we settle on here--but I see that's unrelated to your change.

  4. Note that even in our pathological case we could still just optionally yield

  5. Ah, maybe this predicate is a blocker for just yielding products?

    1. Yes. This wall fall out of the full new engine change it was out of scope for this more limited bandaid on the current engine.

  6. I know it's just rearranging, but this should definitely call super

    1. I already pushed back with Benjy's suggestion of said-same and only updated prepares failing to call super for the cases where Task was not the base class.

    2. Okay sounds good

  7. Ditto super

  8. 
      
MA
  1. This removed the inits from deferred_sources_mapper and aapt_builder?

    1. Looks like they were no-ops.

    2. Thanks. I have a change coming where I can just follow this lead then.

  2. 
      
Loading...