Implementing scoped dependencies and classpath intransitivity.

Review Request #3582 — Created March 17, 2016 and submitted

gmalmquist
pants
gmalmquist/provided-dependencies
3049
pants-reviews
benjyw, nhoward_tw, patricklaw, stuhood, zundel

This adds two fields to all targets: scope and _transitive.

These control how downstream tasks construct the set of targets
used for generating the classpath (in the case of JVM-related
tasks). This can be used to control similar behavior for other
(non-JVM) tasks, but this patch does not attempt to implement
all possible uses.

It is up to the individual task implementations to read and respect
these scopes. This can be facilitated by a couple utility methods,
such as BuildGraph.closure() and Target.closure(), which now take
in include_scopes, exclude_scopes, and respect_intransitive
parameters. The default values for all three of these parameters
results in the previously existing behavior of closure().

This patch modifies jvm tasks so that jvm compiles, runs, binaries,
and bundles respect scopes.

This adds a small number scopes to start with; more can easily be
added. See the Scopes class, where these are defined as constants.

  • DEFAULT - used all the time
  • COMPILE - used only at compile time
  • RUNTIME - used only at run-time (test, run, binary)
  • TEST - used only at test-time

It is possible to use these scopes and the transitive flag to
emulate the "provided" scope found in other build systems (such as
Maven, Gradle, and IntelliJ) like so:

```
jar_library(name='my-provided-target',
jars=[...],
# Only available at test-time and compile-time.
scope='compile test',
)

java_library(name='lib',
dependencies=[
intransitive(':my-provided-target'),
],
)
```

For convenience, when applying a scope to a target is done rarely
or inconsistently, it may be less verbose to use the scoped()
alias like so:

```
jar_library(name='my-library',
jars=[...],
)

java_library(name='lib',
dependencies=[
scoped(':my-library', scope='compile test'),
],
)
```

For the common pattern of "provided" dependencies (targets which
are intransitive and are scoped to 'compile test'), there is a
provided alias:

```
jar_library(name='my-target',
jars=[...],
)

java_library(name='lib',
dependencies=[
provided(':my-target'),
],
)
```

This alias simply constructs an intermediary target with the
appropriate scope and transitive arguments, and returns its address
for inclusion in a dependencies list.

Added tests to:

  • tests/python/pants_test/backend/jvm/tasks/test_scope_provided_integration.py
  • tests/python/pants_test/backend/jvm/tasks/test_scope_runtime_integration.py
  • tests/python/pants_test/build_graph/test_scopes.py

CI went green here: https://travis-ci.org/pantsbuild/pants/builds/116735387
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/117721979
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/117806645
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/117838860
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/118071196
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/118090893
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/118574429
CI went green here: https://travis-ci.org/pantsbuild/pants/builds/119017118

  • 0
  • 0
  • 51
  • 1
  • 52
Description From Last Updated
BE
  1. Is "scope" the ideal name for this concept? I ask because we already use that term in another context, i.e., option scopes. Or is this already-used terminology elsewhere?

    1. The name came from Maven: https://maven.apache.org/pom.html#Dependencies

      scope:

      This element refers to the classpath of the task at hand (compiling and runtime, testing, etc.) as well as how to limit the transitivity of a dependency. There are five scopes available:
      - compile - this is the default scope, used if none is specified. Compile dependencies are available in all classpaths. Furthermore, those dependencies are propagated to dependent projects.
      - provided - this is much like compile, but indicates you expect the JDK or a container to provide it at runtime. It is only available on the compilation and test classpath, and is not transitive.
      - runtime - this scope indicates that the dependency is not required for compilation, but is for execution. It is in the runtime and test classpaths, but not the compile classpath.
      - test - this scope indicates that the dependency is not required for normal use of the application, and is only available for the test compilation and execution phases.
      - system - this scope is similar to provided except that you have to provide the JAR which contains it explicitly. The artifact is always available and is not looked up in a repository.

      I don't think I would have too much of a problem disambiguating option scope concept from the dependency scope concept

    2. I should note that I'm making a couple departures from maven's notion of scopes.

      (1) The 'compile' in this classpath indicates things that are not available at runtime (ie they are only available at and before compile time).
      (2) The 'provided' in this classpath is equivalent to the scopes 'compile' and 'intransitive', but not 'test'. Since tests are separate targets in pants, there isn't really any reason to make 'test' a scope, because we can just only depend on 'test' dependencies from tests.

    3. ... I don't know why I said "classpath"; I meant "patch" >.> I must be tired this morning.

    4. Also, once we decide what we really want, we also need to consider how this will impact the export json format. We need this information for generating IntelliJ configuration.

    5. Ah, yes.

    6. If 'scope' is the term of art, then that's fine with me.

  2. 
      
BE
  1. 
      
  2. Note: My pending list options change gets rid of this niggle entirely.

  3. It's odd that this is an entirely different implementation than closure(), even though it appears (naming-wise, anyway) to be a more general case of it. Can they be merged?

    1. They could be. I wasn't sure whether that was desirable.

    2. I was a little trigger-shy of changing the implementation of all current uses of target.closure() and related functions, since that could potentially have a performance impact on pretty much everything.

    3. I think we should look into this a little deeper maybe? At the very least this requires significant documentation explaining why we have two unrelated implementations of similar functionality...

    4. I had a patch to make both use the same implementation, but I haven't put it up because Stu's concerns require rewriting most of the patch anyway.

  4. 
      
ZU
  1. I wanted to just capture a discussion that Garrett and I had that I think is pretty important.

    If we implement scoped dependencies with a different argument, we have to some how descide how to order dependencies and scoped_dependencies on the classpath. Since they are in two different lists, it seems to follow that we'll either have to construct a classpath with:

    [ scoped_dependencies ] + [ dependencies ]
    

    or

    [ dependencies ] + [ scoped_dependencies ]
    

    It is hard to argue that either one is always right or wrong, its just a decision we would have to make.

    In maven, the scoping mechanism is an attribute you can place on any dependency which gives you the ability to precisely specify the order you want artifacts to appear on the classpath. If we were to do this in pants, it would look like:

    dependencies = [ "dep1", scoped("dep2"), "dep3" ]
    

    I think this is an important issue to consider, because the entire reason for using scoped dependencies is becase you have some weird requirement. It stands to reason that devs would want as much control as possible.

    1. To be clear, this patch implements [ scoped_dependencies ] + [ dependencies ], however a scoped dependency with scope '', None, or 'default' is equivalent to a normal dependency, so if the ordering matters you can just put everything in scoped_dependencies.

  2. 
      
NH
  1. Before I dig deeply into this, could you add some notes about the performance impact of this change?

    1. Is there a particular benchmark you're interested in? Maybe how long BuildGraph.scoped_closure vs BuildGraph.closure takes to run on the whole pants repo?

    2. There is a very slightly slowdown on the performance of running closure() over all the targets in the pants repo -- this implementation appears to be about 7 milliseconds slower per 1000 targets:

      Before: Average time to compute target closure per 1000 targets: 0.00959734466916 secs
      After: Average time to compute target closure per 1000 targets: 0.0167451810914 secs

    3. I ran the benchmark again after all the new changes. Got almost identical timings:

      on master: Average time to compute target closure per 1000 targets: 0.00918402634561 secs
      my branch: Average time to compute target closure per 1000 targets: 0.0101002963014 secs

  2. Rather than using the class attributes as a lookup table, how would you feel about having an explicit lookup table?

    1. I have them as class attributes so that they're easy to reference elsewhere in code (ie Scopes.RUNTIME). I can also put them in a dict, but it would a (admittedly small) bit of extra code duplication to maintain.

  3. I don't like that we parse here. This is called once for every target in the graph and then once again for each of its scoped dependencies.

    Could we push parsing out to the edges and use an enum pattern for these?

    1. Probably; it'll just require a bit more care when we use in_scope in new places.

      What if I memoize parse?

    2. Actually I think I can get a good balance of convenience, performance, and safety by making Scopes.in_scope an instance method and making the Scopes class itself represent a set of scopes rather than using strings.

  4. 
      
ST
  1. As discussed in slack, I think there are two things to consider carefully here.

    One: whether it's a good idea to have scopes independent of transitivity, and included on deps themselves. IE, if I set a jar_library/java_library as compile/runtime/deploy scoped, it would be scoped that way for all libraries/binaries/tests that depend on it, transitively.

    Two: how this interacts with strict_deps, and whether having deps that are explicitly marked intransitive_deps=[..] would make more sense to solve the duped-classfile problem.

    1. One: I really don't like that idea, because it will necessitate having many copies of jar_library targets if you need different scopes in different situations.
      Two: If you have strict_deps on, marking any targets as intransitive is redundant.

    2. I'm working now on switching the syntax to have the scope as a property of target, and to have intransitive as a separate flag.

    3. One: I really don't like that idea, because it will necessitate having many copies of jar_library targets if you need different scopes in different situations.

      If transitivity is split out, what is an example of a case where you would need multiple copies of a jar_library?

      Two: If you have strict_deps on, marking any targets as intransitive is redundant.

      No: because strict_deps only affects compiletime.

    4. Two: Ah, okay. Well then that is still the case -- strict_deps being on will make 'intransitive' redundant at compile-time.

      One:

      jar_library(name='gson',
      jars=[
      jar(org='com.google.code.gson', name='gson', rev='2.3.1'),
      ],
      scope='runtime',
      )

      jar_library(name='gson-2',
      jars=[
      jar(org='com.google.code.gson', name='gson', rev='2.3.1')
      ],
      )

      java_library(name='needs-gson-at-runtime',
      dependencies=[':gson'],
      )

      java_library(name='needs-gson-always',
      dependencies=[':gson-2'],
      )

    5. I updated with a change. It mostly works the way you've suggested, let me know what you think.

    6. The gson example doesn't make sense to me... are you suggesting that in some cases it would be harmful to have it on the classpath at compiletime, and in other cases it wouldn't?

      Keep in mind that if gson was a dependency of an annotation processor, it would be the the annotation processor marked for compile time only, and not gson.

    7. Sorry, the choice of Gson as a library and the particular scopes were arbitrary. Just demonstrating that if you wanted to depend on the same target with two different scopes, and the scopes ignore the structure of the build graph, you end up with ugliness.

    8. Perhaps a better example:

      jar_library(name='gson',
        jars=[...],
      )
      
      annotation_processor(name='aop',
        sources=[...],
        dependencies=[':gson'],
        scope='compile',
      )
      
      # This library will have aop and gson at compile-time, but not runtime.
      # This means that, if we build a binary from this library, we won't 
      # pollute it with the annotation processing library or Gson's classes.
      java_library(name='processed-library',
        sources=[...],
        dependencies=[':aop'],
      )
      
      # This library will have aop at compile-time, but not runtime. It will have
      # Gson both at compile time and runtime, because of a diamond dependency.
      java_library(name='library-that-does-json-stuff',
        sources=[...],
        dependencies=[
          ':gson',
          ':aop',
        ],
      )
      
  2. 
      
GM
ST
  1. A few great cleanups in here! scopes in particular look pretty solid, although I think intransitive vs exclude still needs some thought.

  2. Two find-replace issues in this file.

    1. Whoops. IJ is too aggressive sometimes.

  3. These should be in src/python/pants/backend/jvm probably?

    1. I'm not sure. I'm told there was interested at the pants summit at using scopes for more things than just jvm targets. This is just the first use-case, so to speak.

    2. To be clear: the idea of scopes in general living on Target makes sense to me, as they are likely to be useful in other cases. But the particular constants for compile/runtime etc probably make more sense in jvm-land.

    3. They may also have utility in other languages though, and we may add Scopes beyond just compile/runtime.

  4. src/python/pants/build_graph/build_graph.py (Diff revision 2)
     
     
     
     
     
     

    Worth deprecating probably.

  5. src/python/pants/build_graph/target.py (Diff revision 2)
     
     
     
     

    The semantics of respect_intransitive don't seem quite right... the dep would still be included if any target in the context had a non-intransitive dep on the target... not just the roots. Right?

    I was thinking of intransitive as only affecting a single edge in the graph, rather than the entire context.

    1. You can use scoped('spec', intransitive=True) for that ;-) This approach is more general.

      I am open to changing the name of the alias from scoped to conditional or something else if you think it makes more sense.

  6. src/python/pants/build_graph/target.py (Diff revision 2)
     
     

    Should use a deque rather than a list probably.

    1. Yeah, I'm actually about to push a change that switches things back to use the previously existing graph searches as much as possible, to iron out subtle ordering inconcistencies that CI caught.

  7. This seems useful for intransitive (as currently factored), but it doesn't seem useful for scoped deps, as mentioned earlier.

    As you demonstrated with your aop/gson example, there will generally not be need for indirection for scopes.

    One more thing: intransitive is definitely related to JvmTarget(.., exclude=[]) (ie, a "mid-graph" exclude). Is it possible that refactoring exclude to exclude a target address would be cleaner than intransitive?

    1. In my experience, I've found excluding things after they get in to the classpath to be much more of a pain than stopping them from getting in the first place.

      I think we have opposite ideas of which places the scoped() alias is useful, so I think keeping it in is a decent middle ground - it supports both of our preferences.

    2. In my experience, I've found excluding things after they get in to the classpath to be much more of a pain than stopping them from getting in the first place.

      Yea, quite right. It would be good to attempt to implement one in terms of the other if possible though (or maybe just deprecate mid-graph excludes, I suppose)... the existing excludes implementation is pretty error prone.

      I think we have opposite ideas of which places the scoped() alias is useful, so I think keeping it in is a decent middle ground - it supports both of our preferences.

      To be 100% clear: none of your examples have shown a need for indirection for scope... am I missing something?

    3. There isn't a necessity for it, just as there isn't a necessity for the indirection with intransitive. It'll just be more convenient for us to use constructs like scoped(':my-target', scope='compile test', transitive=False) in places.

    4. I'd be up for refactoring JvmTarget(exclude=...) to use intransitive somehow, or deprecating it, but I'd rather do that in a separate patch, as this one is already pretty substantial and I haven't yet looked into how exclude= is currently implemented.

  8. 
      
GM
GM
ST
  1. Garrett: how would you feel about landing scopes in this review, and splitting out a separate review for transitivity? scopes look fine, and make conceptual sense. But transitivity seems like a thing that might deserve it's own deps list potentially ("intransitive_deps=[..]" as you mentioned, or an inverse of bazel's "exported_deps" perhaps?).

    1. Per discussion in slack, making the transitivity bits experimental here works for me.

      Please add documentation here about the usecases for disabling transitivity for a dep.

  2. I can't really imagine a usecase that would use the leveled_predicate with anything other than 0 as the condition... would this maybe be cleaner as one of the existing predicates with a filter(t) if t not in roots check?

    1. It is very tricky (if possible at all) to use the existing predicates, because of diamond dependencies. The normal flow of the search goes:

      (1) get next vertex
      (2) if vertex has been visited, skip
      (3) mark vertex as visited
      (4) if vertex passes predicate, add to closure and traverse dependencies

      The trouble is that the intransitive property cares about where the vertex is expanded (ie its depth) in addition to the identity of the target. So step (2) will be overly strict in the case of diamond dependencies. E.g., for the graph of [A, B, C] where A has a dep on B and C, and B has an intransitive dep on C, and we search from the roots [A]. If we DFS and see B->C before A->C, we will erroneously skip C, mark it as visited, and never consider it again.

      Which is why I added the leveled_predicate to act just before adding a dependee; that way it will consider the same target multiple times if necessary.

  3. src/python/pants/docs/dev_tasks.md (Diff revision 4)
     
     

    Scopes**

    1. Did I ever mention to you guys how one time I tried to rename a method from add to put in one of my classes for a pants patch, and it renamed every instance in all classes in pants, and in the virtual environment, and in my python installation under /usr/local/lib/python2.7 (including the set class)? It was horrible. I had to reinstall everything.

  4. 
      
GM
GM
GM
ZU
  1. I am not sure where to interject this into the conversation, but in a F2F discussion with Garrett, we discussed the logical splitting of intransitive and scoped, which is fine by me, but there is a very strong use case for having both of them on the same dependency (namely, how provided works in maven)

    If it is possible without too much back bending, I would love to have an alias for 'provided()' to wrap a dependency so that users don't have to get their head around having to configure intransitive and scoped seperately, so that this will directly map to a maven or intelliJ module config.

  2. 
      
GM
GM
ZU
  1. The patch descripion mentions the 'intransitive(' alias and there is mention of it in comments but I don't see any examples of it in BUILD files.

    Also, I know you have some examples in pydoc, but those aren't currently reflected in the generated documentation. I think it would be worth a mention adding these new aliases in http://pantsbuild.github.io/build_files.html

    1. Hmm, yeah it looks like I replaced all instances of intransitive() with provided. I can go back and add a couple.

      I'm not sure about adding to build_files. I think Stu would like to not make them too obvious since they're marked as experimental?

  2. postorder is missing from this list. With the introduction of leveled_predicate, It seems a bit arbitrary whether you declare and pass down an arg or let it be passed through implicitly through **kwargs.

    1. I didn't add the postorder parameter to this method, or change its docstring. I don't know whether its exclusion from the docstring is intentional (due to it already being documented in the method it delegates to), or whether it was an oversight.

      It does seem arbitrary, but that seems to be the status-quo. See the current code: https://github.com/pantsbuild/pants/blob/master/src/python/pants/build_graph/build_graph.py#L272

      I can go and convert everything to use **kwargs, if you think that'd be better?

    2. If it is supposed to be an API method, I think that documenting the parameter is multiple places would be the best (the API should remain stable)

  3. 
      
GM
ZU
  1. Ship It!
  2. 
      
ST
  1. Thanks. Nick should be back tomorrow; please wait for his shipit.

  2. src/python/pants/backend/jvm/tasks/jvm_task.py (Diff revision 7)
     
     
     
     
     

    These are lists of scopes.

    1. No, they're just Scope objects. Though the code would work if you passed in a list of scope names like ['default', 'compile'], that isn't recommended.

  3. src/python/pants/build_graph/intransitive_dependency.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     

    I believe that this will fail if two targets in the same directory have the same scoped/intransitive dep on a particular dep.

    Should either check for the existence of the object, or give each call a unique name.

    1. They do each have a unique name. That's what the __index does.

    2. I'll add a test for this though.

  4. Please also mark experimental.

  5. src/python/pants/build_graph/target_scopes.py (Diff revision 7)
     
     
     
     

    Please mark experimental.

    Alternatively, it sounds like Square will primarily be using the provided() wrapper. If you guys have no usecase for this wrapper, I'd suggest dropping it until we see a usecase.

    1. Ok. We may use scoped('...', scope='compile test') and scoped('...', scope='runtime') also

  6. 
      
GM
NH
  1. 
      
    1. This looks much better. I've got a good number of nitpicks, and a couple larger questions.

  2. nit: add BUILD dep

  3. nit: add as a BUILD dep

  4. This could override classpath to set the include_scopes instead, to reduce some of the duplication.

  5. This implicitly sets respect_intransitive to False, but below we set it to True via the classpath method. I think they should have the same setting.

    1. These both use the classpath method though. How is it implicitly False here?

    2. You're right. Sorry about that.

  6. nit: add build_graph as a BUILD dep.

  7. Can you think of a way to encapsulate the duplicated traversal args used in these tasks? It looks like each task will always use the same args even if it's calling through different methods. It'd be nice if that were clearer.

    1. I could clean things up a bit by just using a convention of sticking the scope in a class variable for reuse, but I'm not sure of an elegant way of getting Target.closure to use default values inferred from the current Task without doing hacky things.

    2. hmm. I think you could also use a fn that takes a fn and executes it with the right scope arguments. I'm fine with figuring out a convention later. My concern is that these call sites are coupled, but it might not be clear by inspection later that they need to have the same arguments.

    3. I think we do need an abstraction around the scoping requirements for traversals for invalidation and fingerprinting as well, but that should be a follow on to this change.

      Since these changes will reduce the transitive graph, having invalidation take advantage of them could result in a neat reduction in cache misses.

    4. On useful sanity check on these APIs (the level_predicate one in particular) is whether they can be used to replace https://github.com/pantsbuild/pants/blob/ff02fbdfe1047ef6356255834b906a7c753589bf/src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py#L565-L584

  8. nit: switch order of include / exclude to reflect precedence.

  9. Please add a note to look at closure_for_targets for arguments. Since this is public, we need to be sure that its kwargs are documented.

  10. I'd like tests for this kwarg. From inspection, it looks like it works slightly different than documented.

    As implemented, it passes (dep, current_target_level), but I think the documentation implies (current_target, current_target_level).

    1. Yeah, I found it a little tricky to describe in the docs.

  11. nit: ws around +

  12. Same note as above wrt leveled_predicate.

  13. nit: ws around +

  14. src/python/pants/build_graph/target.py (Diff revision 8)
     
     

    This should note what happens when the scope is defaulted.

  15. Update to reflect the current behavior.

    It returns a set of scope strings if the passed scope is not empty or blank otherwise it returns a tuple containing only 'default'

  16. I think it'd make sense to order these by precedence, so excludes first.

  17. nit: add are before in

  18. src/python/pants/build_graph/target_scopes.py (Diff revision 8)
     
     
     
     
     

    I'd like some tests for these cases.

  19. src/python/pants/goal/context.py (Diff revision 8)
     
     

    Since this is public, I think it should be explicit in the kwargs it supports.

    I'd like to see we either
    - make Target.closure_for_targets public and note in the docstring here that closure_for_targets contains the remaining options
    - or, explicitly spell out the kwargs in the signature here and duplicate the docs.

  20. It seems strange to me that we have multiple copies of Common. I'm not sure why each target here uses the common it uses.

    eg, one uses two's common, two uses three's and three uses one's.

    Also, why does test depend on one and three's common, but two's shadow? Which common do you expect to win? Maybe the common's should produce differing output?

    Could you reduce the number of them, or document why there are different ones?

    1. It's mostly to show that you can mix and match classes with the same names.

    2. Could you document that with comments or something then? The shadowed names make sense, but I found it confusing for the "common" case.

    3. Sure thing.

  21. tests/python/pants_test/backend/jvm/tasks/test_scope_runtime_integration.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These two tests test both the jar contents and the effect of running the jar, but that's not clear from their names. Could you note that somehow?

  22. Could you add a docstring here explaining why the binary fails to run? I think that would be helpful to explain these tests without having to read through the associated testproject BUILD files.

    Alternatively, naming the targets a little more explicitly, like the runtime integration tests, would also work for me.

  23. I think this should just use the message optional argument rather than changing what is compared.

  24. Do you expect in_scope to ever be called without arguments? I think it'd be better if the args were explicit here rather than defaulted.

    1. It probably won't be called without arguments, but will frequently be called with arguments=None. I can do that explicitly.

    2. Thanks. I think that will make the test case clearer.

  25. Please pull the include/exclude together tests out into a separate case. I think it's important to call those out since they specify the precedence ordering.

  26. I'd like some tests of the type based error cases, to make sure those branches are covered.

  27. tests/python/pants_test/build_graph/test_scopes.py (Diff revision 8)
     
     
     
     
     
     
     

    I feel like these should use OrderedSet or list, since ordering matters for closure_for_targets

    1. I'm not sure -- ordering is extensively tested by other parts of the code base already.

    2. While there are ordering tests elsewhere, scopes change the traversal ordering for interleaved graphs containing scopes that are included/excluded. I'd prefer if these tests covered that change in behavior.

      My note on https://rbcommons.com/s/twitter/r/3582/diff/8?file=3004128#file3004128line86 specifically deals with that. Adding that test might be sufficient.

  28. There are no tests which pass respect_intransitive=False. Add some please.

  29. nit: add line between setup and assertion sections.

  30. nit: add line between setup and assertion sections.

  31. xx, this assertion is also above.

  32. I'd also like to see self.assert_closure({d, c, b_intransitive, a}, [d, c]), where the input order matters.

    1. Good call, that shook out a subtle bug.

  33. nit: add line between setup and assertion sections.

  34. nit: add line between setup and assertion sections.

  35. nit: add line between setup and assertion sections.

  36. nit: add line between setup and assertion sections.

  37. If include_scopes=None, would the result be {a_root, b_root, a, b}? I'd thought that DEFAULT ~= None from the scopes tests.

    1. Yes, None would be {a_root, b_root, a, b}. The input to the include_scopes and exclude_scopes must be Scope objects, they cannot be strings or tuples of strings. So unlike in BUILD files or when constructing a Scope() object (where None is parsed to be Scopes.DEFAULT), None here indicates that there literally is no list of include_scopes, so no filtering based on include_scopes is done. I'll update the docs on Scope.in_scope() to make that more clear.

    2. Thanks.

  38. 
      
GM
GM
NH
  1. Ship it!

    Could you file some issues for folding these concepts into the docs as follow up tasks?

    1. Okay, I'll do this after this patch lands

    2. Filed:
      https://github.com/pantsbuild/pants/issues/3104
      https://github.com/pantsbuild/pants/issues/3105

  2. src/python/pants/build_graph/build_graph.py (Diff revisions 8 - 9)
     
     

    nit: Add a care to "don't about"

  3. src/python/pants/build_graph/build_graph.py (Diff revisions 8 - 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    How would you feel about formulating it this way? I think it makes the preconditions for leveled_predicate more clear.

    walker = self.DepthAwareWalk if leveled_predicate else self.DepthAgnosticWalk
    walk = walker()
    def _walk_rec(addr, level=0):
      target = self._target_by_address[addr]
      if leveled_predicate and level > 0 and leveled_predicate(target, level - 1):
        return
      if not walk.expand_once(target, level):
        return
      if predicate and not predicate(target):
        return
    
      if postorder:
        for dep_address in self._target_dependencies_by_address[addr]:
          _walk_rec(dep_address, level + 1)
    
      if walk.do_work_once(target):
        work(target)
    
      if not postorder:
        for dep_address in self._target_dependencies_by_address[addr]:
          _walk_rec(dep_address, level + 1)
    
    1. Hmm; I like the if X: return pattern in general (I didn't use it here because I was keeping things close to the original code where reasonably possible). However I don't much care for the code duplicating in the recursive for-loops, and I'd rather avoid making the extra recursive call by checking the leveled_predicate before adding dependencies.

      Admittedly, I don't think recursive calls aren't too expensive, but in the bfs case they can be because we'll be adding lots of unnecessary vertices onto the queue before poping them off again -- which I want to avoid because that does noticably impact performance for large graphs.

    2. Fair enough.

  4. src/python/pants/build_graph/build_graph.py (Diff revisions 8 - 9)
     
     
     
     
     
     
     
     
     
     
     
     

    And like this here?

    ordered_closure = OrderedSet()
    # Use the DepthAgnosticWalk if we can, because DepthAwareWalk does a bit of extra work that can
    # slow things down by few millis.
    walker = self.DepthAwareWalk if leveled_predicate else self.DepthAgnosticWalk
    walk = walker()
    to_walk = deque((0, addr) for addr in addresses)
    while len(to_walk) > 0:
      level, address = to_walk.popleft()
      target = self._target_by_address[address]
      if leveled_predicate and level > 0 and leveled_predicate(target, level - 1):
        continue
      if not walk.expand_once(target, level):
        continue
      if predicate and not predicate(target):
        continue
    
      if walk.do_work_once(target):
        ordered_closure.add(target)
      for addr in self._target_dependencies_by_address[address]:
        to_walk.append((level + 1, addr))
    
    return ordered_closure
    
  5. Could you move this to __init__? It's a little confusing to me because the self.__index += 1 in call doesn't increment it, instead creating an instance field that overrides it.

    1. Oh, I didn't realize that. Okay.

  6. I'd prefer if these generated targets used a more specific word in their name than just intransitive in the cases where they are eg provided targets.

    1. I had another thought about this. As currently implemented, I think these will be visible as targets from list et al, I think that means they could be depended on by name from other targets. eg

      scoped(':foo', scope='my-scope')
      target('awesome', dependencies=[':foo-intransitive-0'])
      

      Is there a way to make it so that they can't be treated as implicitly defined targets with stable names that can be referred to directly?

    2. I'm not sure. Giving them instable names would cause lots of cache thrash.

    3. Could we mark them as synthetic? Then they wouldn't show up in ./pants list ....

    4. Is there an easy way to do that? is_synthetic is implemented purely by checking the address type. I guess I could make a new target type that "pretends" to be synthetic (though it'd be weird because you could technically hand-write it into a BUILD file) by overriding the property, but I'm not sure if that would break things or not.

      I think it's unlikely that people will try to reference the targets directly, since (1) it's very obviously a weird and hacky thing to do, and (2) you can just make an explicit target and give it a scope parameter.

    5. I'm not sure if there is an easy way to do that right now. I do think we need some way of making targets at BUILD file parse time that are not visible by default for things like this. I don't think it needs to be the responsibility of this patch to figure that out, but I do think it's a problem.

      My thinking on this is that if something is exposed through the UI, it will be used, or at least we have to treat it as though it will be.

    6. I'll just add -unstable- to their address name for now and a code comment.

    7. Works for me for now.

  7. Same request as with the intermediate factory.

  8. 
      
GM
GM
GM
Review request changed

Status: Closed (submitted)

Change Summary:

In commit b36d444bcd825f1b71eba0dfea5b291ff4c9044c. Thanks for the reviews Benjy, Eric, Stu, and Nick! (And thanks for working so diligently to hammer out the design with me Stu).

Loading...