Nick Howard (Twitter)

unread,

Nov 18, 2016, 3:56:16 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Bugs: 4007, 4076

Repository: pants

Description

Re-implementing trace was deferred during the initial work on the Rust scheduler. This patch partially reintroduces it. The rust implementation will now produce traces that include the subjects and some of the information about failures. Some types of failures will not have good messages yet. This is because we haven't landed a patch to fix https://github.com/pantsbuild/pants/issues/4025. Also, the trace function will produce traces for each of the roots.
Example output:
$ echo "WUT" >> BUILD.tools 
$ ./pants --enable-v2-engine list //:
Exception caught: (<class 'pants.build_graph.address_lookup_error.AddressLookupError'>)
  File "/.../pants/src/python/pants/bin/pants_exe.py", line 50, in <module>
    main()
  File "/.../pants/src/python/pants/bin/pants_exe.py", line 44, in main
    PantsRunner(exiter).run()
  File "/.../pants/src/python/pants/bin/pants_runner.py", line 57, in run
    options_bootstrapper=options_bootstrapper)
  File "/.../pants/src/python/pants/bin/pants_runner.py", line 46, in _run
    return LocalPantsRunner(exiter, args, env, options_bootstrapper=options_bootstrapper).run()
  File "/.../pants/src/python/pants/bin/local_pants_runner.py", line 37, in run
    self._run()
  File "/.../pants/src/python/pants/bin/local_pants_runner.py", line 77, in _run
    self._exiter).setup()
  File "/.../pants/src/python/pants/bin/goal_runner.py", line 184, in setup
    goals, context = self._setup_context()
  File "/.../pants/src/python/pants/bin/goal_runner.py", line 157, in _setup_context
    self._daemon_graph_helper
  File "/.../pants/src/python/pants/bin/goal_runner.py", line 101, in _init_graph
    graph, address_mapper = graph_helper.create_build_graph(target_roots, self._root_dir)
  File "/.../pants/src/python/pants/bin/engine_initializer.py", line 88, in create_build_graph
    for _ in graph.inject_specs_closure(target_roots.as_specs()):
  File "/.../pants/src/python/pants/engine/legacy/graph.py", line 205, in inject_specs_closure
    for address in self._inject(specs):
  File "/.../pants/src/python/pants/engine/legacy/graph.py", line 225, in _inject
    self._index(target_entries)
  File "/.../pants/src/python/pants/engine/legacy/graph.py", line 83, in _index
    'Build graph construction failed for {}:\n{}'.format(node, trace))

Exception message: Build graph construction failed for (SiblingAddresses(directory=u''), Select(Collection.of(HydratedTarget))):
Computing =Collection.of(HydratedTarget) for SiblingAddresses(directory=u'')
  Computing =Collection.of(HydratedTarget) for SiblingAddresses(directory=u'')
    Computing =HydratedTarget for SiblingAddresses(directory=u'')
      Computing =Collection.of(Address) for SiblingAddresses(directory=u'')
        Computing =Collection.of(Address) for SiblingAddresses(directory=u'')
          Computing =AddressFamily for SiblingAddresses(directory=u'')
            Computing =AddressFamily for Dir(path=u'')
              Computing =AddressFamily for Dir(path=u'')
                Throw("EntryId(17) failed!")

Computing =Collection.of(Address) for SiblingAddresses(directory=u'')
  Computing =Collection.of(Address) for SiblingAddresses(directory=u'')
    Computing =AddressFamily for SiblingAddresses(directory=u'')
      Computing =AddressFamily for Dir(path=u'')
        Computing =AddressFamily for Dir(path=u'')
          Throw("EntryId(17) failed!")

Testing

Local manual testing with ./pants list and a corrupted BUILD file. Running tests that had been skipped due to this feature being missing. CI away on https://github.com/pantsbuild/pants/pull/4076

Diffs

src/python/pants/backend/python/tasks2/resolve_requirements.py (363a89488edde95722a67570e8ed17c3293c0040)
src/python/pants/engine/legacy/graph.py (27ad3717fc4f3a51101fd1a5adec3c97c18fefc8)
src/python/pants/engine/scheduler.py (44f583aadf5990979ae03bde21418edad16db742)
src/python/pants/engine/subsystem/native.py (2d305fd6e79766767adcf5faf759e3a5ccf9a96c)
src/python/pants/engine/subsystem/native_engine_version (f5d64efdb5fc77be45e23bd42d049c010e2e2ea9)
src/rust/engine/src/graph.rs (5c7419221dc6a45793bdef32afd1a3f1484c2670)
src/rust/engine/src/lib.rs (504f23137e9fb1330f26fff1ac3a3e457aca4c46)
src/rust/engine/src/scheduler.rs (a6738cc6f21a43f958c0c243bbc05824d71a09cf)
tests/python/pants_test/backend/python/tasks2/test_resolve_requirements.py (87151201c5342178cc6c687c8a1d36a4d0feb94d)
tests/python/pants_test/engine/legacy/test_build_ignore_integration.py (c3b7a37197e8d6e83dd430353973a3a5b186cd49)
tests/python/pants_test/engine/legacy/test_filemap_integration.py (95104e18874d3c92d72ab20400c84381ff44aaf9)
tests/python/pants_test/engine/test_build_files.py (3321240b8097bd7edc019b4cf9bac19b4961c3ea)

View Diff

Stu Hood

unread,

Nov 18, 2016, 4:47:29 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

Fix it, then Ship it!

Thanks Nick!

src/python/pants/engine/scheduler.py (Diff revision 1)
212	def trace(self, roots):	213	def trace(self, roots):

The roots arg is unused now, which is (IMO) fine. But should remove it, or replace it with passing in and validating the request similar to what def root_entries does.

src/python/pants/engine/subsystem/native.py (Diff revision 1)
		137	char* graph_trace(RawScheduler, char);

This is now a void function.

src/rust/engine/src/graph.rs (Diff revision 1)
261	<</**	262	/**
		263	* Begins a topological walk from the given roots. Provides both the current entry as well as the
		264	* depth from the root.
		265	*/

two-space indentation probably

src/rust/engine/src/graph.rs (Diff revision 1)
		427	for _ in 0..level {
		428	indent = indent + " ";

Maybe better with indent.push(' ')?

src/rust/engine/src/graph.rs (Diff revision 1)
		437	match entry.node.product() {
		438	ref x => format!("{}", externs.id_to_str(x.0))
		439	},
		440	match entry.node.subject() {
		441	ref x => format!("{}", externs.id_to_str(x.id()))
		442	});

If you're trying to ref/deref something, you can just use the * or & operators, similar to C/C++. So I think each of these matches should just be:

externs.id_to_str(*entry.node.product())

Also, you shouldn't need the second format! call in here, since the result is already a string.

src/rust/engine/src/graph.rs (Diff revision 1)
		444	format!("{}\n{} {}", output, indent, match entry.state {
		445	Some(Complete::Return(ref x)) => format!("Return({})", externs.val_to_str(x)),
		446	Some(Complete::Throw(ref x)) => format!("Throw({:?})", x),
		447	Some(Complete::Noop(ref x, ref opt_node)) => format!("Noop({:?}, {:?})", x, opt_node),
		448	None => String::new(),
		449	})

Not a fan of the inline match like this, as opposed to breaking it out and naming it. But minor.

src/rust/engine/src/lib.rs (Diff revision 1)
		459	let path_str = unsafe { CStr::from_ptr(path_ptr).to_string_lossy().into_owned() };
		460	let path = Path::new(path_str.as_str());
		461	with_scheduler(scheduler_ptr, \|raw\| {

two-space indent

- Stu Hood

On November 18th, 2016, 8:56 p.m. UTC, Nick Howard (Twitter) wrote:

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Updated Nov. 18, 2016, 8:56 p.m.

Nick Howard (Twitter)

unread,

Nov 18, 2016, 6:03:25 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

On November 18th, 2016, 2:47 p.m. MST, Stu Hood wrote:
src/python/pants/engine/scheduler.py (Diff revision 1)
212
  def trace(self, roots):
213
  def trace(self, roots):
The roots arg is unused now, which is (IMO) fine. But should remove it, or replace it with passing in and validating the request similar to what def root_entries does.

I think I'll just rm it for now.

On November 18th, 2016, 2:47 p.m. MST, Stu Hood wrote:
src/rust/engine/src/graph.rs (Diff revision 1)
427
      for _ in 0..level {
428
        indent = indent + "  ";
Maybe better with indent.push(' ')?

Neat. I went with push_str.

On November 18th, 2016, 2:47 p.m. MST, Stu Hood wrote:

src/rust/engine/src/graph.rs (Diff revision 1)
		437	match entry.node.product() {
		438	ref x => format!("{}", externs.id_to_str(x.0))
		439	},
		440	match entry.node.subject() {
		441	ref x => format!("{}", externs.id_to_str(x.id()))
		442	});

If you're trying to ref/deref something, you can just use the * or & operators, similar to C/C++. So I think each of these matches should just be:

externs.id_to_str(*entry.node.product())

Also, you shouldn't need the second format! call in here, since the result is already a string.

I think I'd had some issues with str vs String in here. Not necessary. Thanks for calling it out.

On November 18th, 2016, 2:47 p.m. MST, Stu Hood wrote:

src/rust/engine/src/graph.rs (Diff revision 1)
		444	format!("{}\n{} {}", output, indent, match entry.state {
		445	Some(Complete::Return(ref x)) => format!("Return({})", externs.val_to_str(x)),
		446	Some(Complete::Throw(ref x)) => format!("Throw({:?})", x),
		447	Some(Complete::Noop(ref x, ref opt_node)) => format!("Noop({:?}, {:?})", x, opt_node),
		448	None => String::new(),
		449	})

Not a fan of the inline match like this, as opposed to breaking it out and naming it. But minor.

Broke it out.

On November 18th, 2016, 2:47 p.m. MST, Stu Hood wrote:
src/rust/engine/src/lib.rs (Diff revision 1)
459
    let path_str = unsafe { CStr::from_ptr(path_ptr).to_string_lossy().into_owned() };
460
    let path = Path::new(path_str.as_str());
461
    with_scheduler(scheduler_ptr, |raw| {
two-space indent

Got it.

- Nick

On November 18th, 2016, 1:56 p.m. MST, Nick Howard (Twitter) wrote:

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Updated Nov. 18, 2016, 1:56 p.m.

Nick Howard (Twitter)

unread,

Nov 18, 2016, 6:08:49 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Updated Nov. 18, 2016, 4:08 p.m.

Changes

Follow up for Stu's comments.

Diffs (updated)

src/python/pants/backend/python/tasks2/resolve_requirements.py (363a89488edde95722a67570e8ed17c3293c0040)
src/python/pants/engine/engine.py (390fc91618a2c4cf545d155c64d56c0f71758e3b)
src/python/pants/engine/legacy/graph.py (8a561654b55551c69d3b522e168df8f13722606e)
src/python/pants/engine/scheduler.py (bbd8be609c66855c3a6b8cf17ec1b7307a4f1c77)
src/python/pants/engine/subsystem/native.py (4e3adc3e08b3344f309c0fc79a58f8c682d9b17b)
src/python/pants/engine/subsystem/native_engine_version (a3bb7251fd7ed0da49179fa21510fb8d17f56fb2)
src/rust/engine/src/graph.rs (5ea55585790178244d28786a6a38a53b89e0f21d)
src/rust/engine/src/lib.rs (fb3d67c9eb9b9f5f28b8be1ab3e612694a0f0256)

src/rust/engine/src/scheduler.rs (a6738cc6f21a43f958c0c243bbc05824d71a09cf)
tests/python/pants_test/backend/python/tasks2/test_resolve_requirements.py (87151201c5342178cc6c687c8a1d36a4d0feb94d)
tests/python/pants_test/engine/legacy/test_build_ignore_integration.py (c3b7a37197e8d6e83dd430353973a3a5b186cd49)
tests/python/pants_test/engine/legacy/test_filemap_integration.py (95104e18874d3c92d72ab20400c84381ff44aaf9)
tests/python/pants_test/engine/test_build_files.py (3321240b8097bd7edc019b4cf9bac19b4961c3ea)

tests/python/pants_test/engine/test_isolated_process.py (1014f674d64c338c216aaf773b35e509f0e60c26)

View Diff

Show Changes

Nick Howard (Twitter)

unread,

Nov 18, 2016, 7:10:37 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Updated Nov. 18, 2016, 5:10 p.m.

Changes

Caught one more trace usage.

tests/python/pants_test/engine/test_build_files.py (4135ce897cf2163d15e3b1ba63302b6e122e4c80)
tests/python/pants_test/engine/test_isolated_process.py (1014f674d64c338c216aaf773b35e509f0e60c26)

View Diff

Show Changes

Nick Howard (Twitter)

unread,

Nov 18, 2016, 7:23:02 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

On November 18th, 2016, 5:15 p.m. MST, Kris Wilson wrote:

src/rust/engine/src/lib.rs (Diff revision 2)
		463	println!("Failed to write trace to {}: {:?}", path.display(), e);

probably fine for the moment, but I suspect raw println's from the native side won't be daemon-safe due to the way the pailgun works (they'll likely end up in the daemons stdout which goes to pantsd.log).

we can circle back to this independently tho.

I had that thought too, but felt it was better to follow an existing pattern to be cleaned up later, than to do something different that might be hard to grep for later.

- Nick

On November 18th, 2016, 5:10 p.m. MST, Nick Howard (Twitter) wrote:

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Updated Nov. 18, 2016, 5:10 p.m.

Bugs: 4007, 4076

Stu Hood

unread,

Nov 18, 2016, 8:04:43 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

On November 19th, 2016, 12:15 a.m. UTC, Kris Wilson wrote:

src/rust/engine/src/lib.rs (Diff revision 2)
		463	println!("Failed to write trace to {}: {:?}", path.display(), e);

probably fine for the moment, but I suspect raw println's from the native side won't be daemon-safe due to the way the pailgun works (they'll likely end up in the daemons stdout which goes to pantsd.log).

we can circle back to this independently tho.

On November 19th, 2016, 12:23 a.m. UTC, Nick Howard (Twitter) wrote:

I had that thought too, but felt it was better to follow an existing pattern to be cleaned up later, than to do something different that might be hard to grep for later.

+1... I'll look at this when I figure out the exception issue.

- Stu

On November 19th, 2016, 12:10 a.m. UTC, Nick Howard (Twitter) wrote:

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Updated Nov. 19, 2016, 12:10 a.m.

Stu Hood

unread,

Nov 18, 2016, 8:04:57 PM11/18/16

to Benjy Weinberger, Stu Hood, Yujie Chen, Kris Wilson, pants-reviews, Nick Howard (Twitter)

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4392/

Ship it!

Ship It!

- Stu Hood

On November 19th, 2016, 12:10 a.m. UTC, Nick Howard (Twitter) wrote:

Review request for pants-reviews, Benjy Weinberger, Kris Wilson, Stu Hood, and Yujie Chen.

By Nick Howard (Twitter).

Updated Nov. 19, 2016, 12:10 a.m.

Bugs: 4007, 4076

Reply all

Reply to author

Forward

Review Request 4392: [engine] Initial Trace implementation for rust engine

Nick Howard (Twitter)

Description

Testing

Diffs

Stu Hood

Nick Howard (Twitter)

Nick Howard (Twitter)

Changes

Diffs (updated)

Nick Howard (Twitter)

Changes

Nick Howard (Twitter)

Stu Hood

Stu Hood