https://pantsbuild.org/ logo
Powered by
#development
  • w

    witty-crayon-22786

    07/02/2018, 11:25 PM
    a trivial implementation of "don't cache this root, but do cache its dependencies" would be to not actually create the root
  • w

    witty-crayon-22786

    07/02/2018, 11:26 PM
    Graph::create
    creates a root in the graph: it assigns an 
    EntryId
    for the node, and then runs it
  • w

    witty-crayon-22786

    07/02/2018, 11:26 PM
    Graph::get
    is used by nodes to request their dependencies
  • w

    witty-crayon-22786

    07/02/2018, 11:27 PM
    (via 
    Context::get
    )
  • w

    witty-crayon-22786

    07/02/2018, 11:27 PM
    so 
    get
    creates an edge between one node and a dependency, creating the dependency if it doesn't exist
  • w

    witty-crayon-22786

    07/02/2018, 11:27 PM
    and 
    create
    just creates a node
  • r

    rough-minister-58256

    07/02/2018, 11:27 PM
    ic
  • w

    witty-crayon-22786

    07/02/2018, 11:28 PM
    ...i'm just dumping info that should be in the docs (but isn't) at this point
  • w

    witty-crayon-22786

    07/02/2018, 11:29 PM
    so, to run a 
    @console_rule
    i see two options: 1) don't actually create a Node in the Graph for it at all, and instead run it with a 
    Context
    that uses 
    Graph::create
    (rather than 
    get
    ) to get dependencies
  • w

    witty-crayon-22786

    07/02/2018, 11:30 PM
    ...which (because there is no Node in the graph) would definitely not cache the 
    @console_rule
    , but would cache the things it depends on
  • w

    witty-crayon-22786

    07/02/2018, 11:30 PM
    or
  • w

    witty-crayon-22786

    07/02/2018, 11:32 PM
    2) do create a Node in the graph for the 
    @console_rule
    , but add explicit support to 
    Graph
    for not recording a value for a Node
  • w

    witty-crayon-22786

    07/02/2018, 11:33 PM
    ...which might look like "never actually holding a 
    Future
    on Entry for that node type"
  • w

    witty-crayon-22786

    07/02/2018, 11:33 PM
    ----
  • w

    witty-crayon-22786

    07/02/2018, 11:34 PM
    (2) is probably a bit better long term, because it is more general. but unfortunately it would definitely collide with what i'm doing in https://github.com/pantsbuild/pants/pull/6013 and https://github.com/pantsbuild/pants/pull/6059
  • w

    witty-crayon-22786

    07/02/2018, 11:36 PM
    ...but (2) might also actually be easier, so it might still be the way to go.
  • w

    witty-crayon-22786

    07/02/2018, 11:36 PM
    i think what it would look like would be adding a method to 
    trait Node
    that is approximately 
    fn memoize(&self) -> bool
    ☝️ 1
  • w

    witty-crayon-22786

    07/02/2018, 11:37 PM
    and in 
    graph/src/lib.rs
    Entry::get
    , if 
    !memoize
    , don't ever actually store a Future for the Node.
    a
    • 2
    • 2
  • w

    witty-crayon-22786

    07/02/2018, 11:39 PM
    ... so yea. i think 2 is probably pretty straightforward. depending whether https://github.com/pantsbuild/pants/pull/6013 or your change lands first, one of us would need to do a little bit of rebasing, but it's small enough that i don't think it would be that bad.
  • r

    rough-minister-58256

    07/02/2018, 11:45 PM
    word
  • r

    rough-minister-58256

    07/03/2018, 1:44 AM
    whats the plan to land #6013?
  • r

    rough-minister-58256

    07/03/2018, 1:45 AM
    just knocking out the final issue there and landing, yeh?
  • w

    witty-crayon-22786

    07/03/2018, 1:48 AM
    @rough-minister-58256: yea... now that i have https://github.com/pantsbuild/pants/pull/6059 out and it is feeling sane/solid, i'm going to focus on getting 6013 in
  • r

    rough-minister-58256

    07/03/2018, 1:48 AM
    metal
  • w

    witty-crayon-22786

    07/03/2018, 1:49 AM
    should be able to land it tomorrow, hopefully.
  • r

    rough-minister-58256

    07/03/2018, 1:49 AM
    saweet
  • w

    witty-crayon-22786

    07/03/2018, 2:27 AM
    hm... if a Node has dependents there is a bit more involved in (2) than i described (but `@console_rule`s don't, so not a big deal there)
  • w

    witty-crayon-22786

    07/03/2018, 2:28 AM
    (basically, every time a non-memoizable 
    Node
    runs, everything above it must be considered dirty according to the definition on 6059)
  • r

    rough-minister-58256

    07/03/2018, 2:31 AM
    ic
  • a

    average-vr-56795

    07/03/2018, 11:47 PM
    @witty-crayon-22786 My comments on https://github.com/pantsbuild/pants/pull/6059 are all pretty minor, other than: 1. Should we remember the last 1 or last 
    generation
    Generations? 2. Should we re-use Errors across Generations Has it changed noticeably since I reviewed it, other than rebasing? Also, is the 
    invalidate_randomly
    comment you left a “the code is broken” comment, or a “the test is broken” comment?
1...551552553...906Latest