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

Design strategy for externalizing Target fields #3560

Closed
stuhood opened this Issue Jun 6, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Jun 6, 2016

One of the largest end-user-visible semantic changes in the engine experiments was the idea of moving from typed Target classes to untyped Targets holding typed "products". And the most beneficial part of that change was moving from typed Targets to targets holding a typed Sources subclass as a product.

This ticket deals with designing a strategy to begin exposing typed Sources from targets, which should begin to erode the need for typed Target subclasses. It blocks #2996, which hopes to move graph-mutation (including codegen/deferred-sources) tasks into the new engine.

In terms of scope, this issue should likely reach far enough down the road to have a reasonable answer to how we might migrate from java_library(.., sources=..) to target(.., java(globs=..)), and whether that would require a major version bump to 2.x.

cc @jsirois, @patricklaw , @benjyw

@stuhood stuhood added the engine label Jun 6, 2016

stuhood added a commit that referenced this issue Jun 28, 2016

[engine] Fix TargetMacro replacements of adapted aliases
In cases where a TargetMacro has been installed as wrapping an existing symbol, it will not exist in the `target_aliases` field, and will thus not be replaced with our adaptor.

"Why yes, this continues to highlight the importance of:"
#3560
#3561

Testing Done:
https://travis-ci.org/pantsbuild/pants/builds/140649055
And tested on internal TargetMacro usecases.

Bugs closed: 3506, 3607

Reviewed at https://rbcommons.com/s/twitter/r/4000/

digwanderlust added a commit to digwanderlust/pants that referenced this issue Jul 1, 2016

Prepare the 1.1.0-rc0 release
  1.1.0-rc0 (7/1/2016)
  --------------------

  This is the first `1.1.0-rc` release on the way to the `1.1.0`.

  New Features
  ~~~~~~~~~~~~

  * Subprocess clean-all
    `RB pantsbuild#4011 <https://rbcommons.com/s/twitter/r/4011>`_

  * expose products for jvm bundle create and python binary create tasks
    `RB pantsbuild#3959 <https://rbcommons.com/s/twitter/r/3959>`_
    `RB pantsbuild#4015 <https://rbcommons.com/s/twitter/r/4015>`_

  * Implement zinc `unused deps` check
    `RB pantsbuild#3635 <https://rbcommons.com/s/twitter/r/3635>`_

  API Changes
  ~~~~~~~~~~~

  * Add `is_target_root` in export
    `RB pantsbuild#4030 <https://rbcommons.com/s/twitter/r/4030>`_

  Bugfixes
  ~~~~~~~~

  * ConsoleRunner bugfix for @TestSerial and other test cleanups
    `RB pantsbuild#4026 <https://rbcommons.com/s/twitter/r/4026>`_

  New Engine Work
  ~~~~~~~~~~~~~~~

  * [engine] Proper implementation of `**` globs in the v2 engine
    `RB pantsbuild#4034 <https://rbcommons.com/s/twitter/r/4034>`_

  * [engine] Fix TargetMacro replacements of adapted aliases
    `Issue pantsbuild#3560 <https://github.com/pantsbuild/pants/issues/3560>`_
    `Issue pantsbuild#3561 <https://github.com/pantsbuild/pants/issues/3561>`_
    `RB pantsbuild#4000 <https://rbcommons.com/s/twitter/r/4000>`_

  Refactoring, Improvements, and Tooling
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  * Fix dead apidocs link for guava.
    `RB pantsbuild#4037 <https://rbcommons.com/s/twitter/r/4037>`_

  * Bump setproctitle to 1.1.10.
    `Issue pantsbuild#44 <https://github.com/dvarrazzo/py-setproctitle/issues/44>`_
    `RB pantsbuild#4035 <https://rbcommons.com/s/twitter/r/4035>`_

  * Set a default read timeout for fetching node pre-installed modules. 1 second default often fails
    `RB pantsbuild#4025 <https://rbcommons.com/s/twitter/r/4025>`_

  * Improve stderr handling for ProcessManager.get_subprocess_output().
    `RB pantsbuild#4019 <https://rbcommons.com/s/twitter/r/4019>`_

  * Add AnnotatedParallelClassesAndMethodsTest* and AnnotatedParallelMethodsTest*
    `RB pantsbuild#4027 <https://rbcommons.com/s/twitter/r/4027>`_

digwanderlust added a commit to digwanderlust/pants that referenced this issue Jul 1, 2016

Prepare the 1.1.0-rc0 release
1.1.0-rc0 (7/1/2016)
  --------------------

  This is the first `1.1.0-rc` release on the way to the `1.1.0`.

  New Features
  ~~~~~~~~~~~~

  * Subprocess clean-all
    `RB pantsbuild#4011 <https://rbcommons.com/s/twitter/r/4011>`_

  * expose products for jvm bundle create and python binary create tasks
    `RB pantsbuild#3959 <https://rbcommons.com/s/twitter/r/3959>`_
    `RB pantsbuild#4015 <https://rbcommons.com/s/twitter/r/4015>`_

  * Implement zinc `unused deps` check
    `RB pantsbuild#3635 <https://rbcommons.com/s/twitter/r/3635>`_

  API Changes
  ~~~~~~~~~~~

  * Add `is_target_root` in export
    `RB pantsbuild#4030 <https://rbcommons.com/s/twitter/r/4030>`_

  Bugfixes
  ~~~~~~~~

  * ConsoleRunner bugfix for @TestSerial and other test cleanups
    `RB pantsbuild#4026 <https://rbcommons.com/s/twitter/r/4026>`_

  New Engine Work
  ~~~~~~~~~~~~~~~

  * [engine] Proper implementation of `**` globs in the v2 engine
    `RB pantsbuild#4034 <https://rbcommons.com/s/twitter/r/4034>`_

  * [engine] Fix TargetMacro replacements of adapted aliases
    `Issue pantsbuild#3560 <https://github.com/pantsbuild/pants/issues/3560>`_
    `Issue pantsbuild#3561 <https://github.com/pantsbuild/pants/issues/3561>`_
    `RB pantsbuild#4000 <https://rbcommons.com/s/twitter/r/4000>`_

  Refactoring, Improvements, and Tooling
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  * Fix dead apidocs link for guava.
    `RB pantsbuild#4037 <https://rbcommons.com/s/twitter/r/4037>`_

  * Bump setproctitle to 1.1.10.
    `Issue pantsbuild#44 <https://github.com/dvarrazzo/py-setproctitle/issues/44>`_
    `RB pantsbuild#4035 <https://rbcommons.com/s/twitter/r/4035>`_

  * Set a default read timeout for fetching node pre-installed modules. 1 second default often fails
    `RB pantsbuild#4025 <https://rbcommons.com/s/twitter/r/4025>`_

  * Improve stderr handling for ProcessManager.get_subprocess_output().
    `RB pantsbuild#4019 <https://rbcommons.com/s/twitter/r/4019>`_

  * Add AnnotatedParallelClassesAndMethodsTest* and AnnotatedParallelMethodsTest*
    `RB pantsbuild#4027 <https://rbcommons.com/s/twitter/r/4027>`_

Testing Done:
CI pending: https://travis-ci.org/pantsbuild/pants/builds/141717353

Bugs closed: 3620

Reviewed at https://rbcommons.com/s/twitter/r/4042/

@stuhood stuhood removed the engine-codegen label Aug 8, 2016

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Aug 11, 2016

This might also allow not requesting sources for tasks that don't use them. With the current implementation, list for example, will evaluate all the source globs.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jan 27, 2017

Getting rid of Target inheritance entirely is not going to happen anytime soon.

But one strategy that allows us to move more pre-computation into the engine is to begin externalizing target arguments, so that the engine can compute more before passing them in... and then, eventually, not pass them in.

I think my strategy will be to define a series of Field instances as class properties of the relevant Target subclass, which the engine can use while hydrating a target to pre-hydrate its fields. This code will likely need to replace Payload (eventually).

peiyuwang added a commit that referenced this issue Mar 1, 2017

[engine] Support implicit sources in v2 engine (#4294)
### Problem

In order to compute source files signature, v2 engine looks for `sources` field, now with 5813a2d , it needs to support implicit sources with `sources` field is not present.

### Solution

The approach in the rb is more short term/cheap to continue to use `TargetAdaptor`, extends this hierarchy to have default source suffixes and excludes there. The alternative/longer term solution is #3560.

### Result

See the new test in `test_graph.test_implicit_sources`, w/o the change it would fail:
```
                           except Exception as e:
                             raise TargetDefinitionException(
                                 target_adaptor.address,
                     >           'Failed to instantiate Target with type {}: {}'.format(target_cls, e))
                     E       TargetDefinitionException: Invalid target BuildFileAddress(testprojects/tests/python/pants/file_sets/BUILD, implicit_sources): Failed to instantiate Target with type <class 'pants.backend.python.targets.python_library.PythonLibrary'>: Unrecognized command line flags on global scope: --color, --confcutdir, --resultlog
                     
                     .pants.d/python-setup/chroots/a447b84a0a5e9517301fc0570a067595fbecdf2f/pants/engine/legacy/graph.py:158: TargetDefinitionException
```

@stuhood stuhood changed the title Design strategy for moving to Sources-as-a-Product Design strategy for externalizing Target fields Mar 16, 2017

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Mar 16, 2017

Retitled to more accurately cover the current goal.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 4, 2017

Relevant to this task: we need to make sure the new design integrates tightly with the --changed options: see #4420 (comment)

@stuhood stuhood added this to the 1.3.0 milestone Apr 10, 2017

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 10, 2017

Bit of a stretch, but I'm targetting this to 1.3.0 so that we can have as much thought as possible put into this before the 1.3.0 milestone, in case there are any obvious deprecations.

@stuhood stuhood removed their assignment Apr 12, 2017

lenucksi added a commit to lenucksi/pants that referenced this issue Apr 25, 2017

[engine] Support implicit sources in v2 engine (pantsbuild#4294)
### Problem

In order to compute source files signature, v2 engine looks for `sources` field, now with 5813a2d , it needs to support implicit sources with `sources` field is not present.

### Solution

The approach in the rb is more short term/cheap to continue to use `TargetAdaptor`, extends this hierarchy to have default source suffixes and excludes there. The alternative/longer term solution is pantsbuild#3560.

### Result

See the new test in `test_graph.test_implicit_sources`, w/o the change it would fail:
```
                           except Exception as e:
                             raise TargetDefinitionException(
                                 target_adaptor.address,
                     >           'Failed to instantiate Target with type {}: {}'.format(target_cls, e))
                     E       TargetDefinitionException: Invalid target BuildFileAddress(testprojects/tests/python/pants/file_sets/BUILD, implicit_sources): Failed to instantiate Target with type <class 'pants.backend.python.targets.python_library.PythonLibrary'>: Unrecognized command line flags on global scope: --color, --confcutdir, --resultlog
                     
                     .pants.d/python-setup/chroots/a447b84a0a5e9517301fc0570a067595fbecdf2f/pants/engine/legacy/graph.py:158: TargetDefinitionException
```
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 25, 2017

Deprioritized #4439 in order to work on this a bit instead, because I believe it's important to have pieces of this and all of #4508 figured out before cutting 1.3.0.

@stuhood stuhood self-assigned this Apr 25, 2017

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 27, 2017

Drafted an initial summary of some concepts in the area, and our goals. Will work on actually proposing what to do to externalize fields tomorrow.

https://docs.google.com/document/d/102EFbwk6cpM9-_4ZSYhMQYA0zL1UKbXzW-DCd8KsFeg/edit?usp=sharing

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 27, 2017

Tiny bit more progress on documenting history, but didn't actually dive into the proposed design... too busy today. Tomorrow!

@stuhood

This comment has been minimized.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented May 1, 2017

LGTM, niggly comment left that's really addressable in code review. The design of schema factory method, instance values method makes good sense to transition.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 1, 2017

Ok... I think this is a plan. Closing this... will follow up in #4535

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment