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

Port IsolatedProcess implementation from Python to Rust - Split 1 #5239

Merged
merged 9 commits into from Jan 20, 2018

Conversation

Projects
None yet
4 participants
@ity
Copy link
Member

ity commented Dec 21, 2017

Problem

From #4397 - Isolated processes currently work for the purposes of testing, but there are lots of abstraction leaks: in particular, a _Snapshots snapshots object is exposed which the python code uses to reverse engineer the location of snapshots.

Instead, process execution should be ported to rust (maybe using duct?) to remove that leak.

Additionally, the outcome of discussion around https://docs.google.com/document/d/1jGq34ds_fhRLPDnmHNV_FHcJiJnbhmk2XEwOf9-w5To/edit#heading=h.ylneg9wnwxcs was that we should adapt the process execution model (which is implicitly also a "remote process execution" model) more closely with the draft of bazel's process execution model.

Solution

This is the first split of the bigger branch (master...ity:ity/cloc_rules) that ports the implementation to Rust. This PR can go in independently and will make it easier to work with smaller branches.
Changes made in this PR:

  • Invoke process execution in Rust from Python
  • Plumbing for ExecuteProcessRequest/ExecuteProcessResult
  • cloc can be invoked from commandline which invoke Rust process execution (thanks to @illicitonion)
  • Test (test_javac_version_example) added in python which invokes Rust through Rules.
  • Removed directories_to_create after discussing with @illicitonion

Result

This PR will follow with:

  • Move directory creation to Rust (currently in Python for testing)
  • Threadpool implementation in Rust (blocking in this PR)
  • Changes on top of #5106 to fix failing tests
@illicitonion
Copy link
Contributor

illicitonion left a comment

Exciting progress! I can run ./pants cloc locally, and the tests all pass, which is great!

Comments are a big handful of really small things to clean up, and a couple of non-PR-blocking things that probably require a little thought/discussion :)

'--skip-uniqueness',
'--ignored={}'.format(ignored_file),
'--list-file={}'.format(list_file),
'--report-file={}'.format(report_file))

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

nit: Wrap the trailing ) on the newline (I know it was like this before, but... :))

This comment has been minimized.

@ity

ity Jan 10, 2018

Member

done

'--ignored={}'.format(ignored_file),
'--list-file={}'.format(list_file),
'--report-file={}'.format(report_file))
if self.context._scheduler is None:

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

If this is becoming a public part of the API, it probably shouldn't be exposed as _scheduler... Maybe we could expose a synchronous_product_request method, so this would look like:

root_entries = self.context.synchronous_product_request([ExecuteProcessResult], [ExecuteProcessRequest(cmd, dict(os.environ))])

and which throws if the ExecutionResult is an error, nicely hiding the awkward details of the ffi API? This is probably something we should do in this PR, because exposing private symbols is sketchy.

I'd also be tempted to give Context an execute_subprocess method which looks something like:

def execute_subprocess(self, execute_process_request):
  if self._scheduler:
    return self.synchronous_product_request([ExecuteProcessResult], [execute_process_request])
  else:
    with self.new_workunit(
      cmd=' '.join(execute_process_request.args),
      ...
    ):
      return ExecuteProcessResult.from_subprocess_result(subprocess.check_call(cmd, ...))

because this is going to be a common thing, but I can believe we'd want to hold off on that until we have a second caller.

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

Let's avoid making it public until we're explicitly tackling #5100. I've transferred these notes there.

request = self.context._scheduler.execution_request([ExecuteProcessResult], [req])
root_entries = self.context._scheduler.execute(request).root_products
if len(root_entries) != 1:
raise Exception("Got {} root entries ({})".format(len(root_entries), root_entries))

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

This should probably be a more specific Exception, and have a slightly more verbose error message for context (mentioning the exact request that went in), because if we end up here something really weird has happened in the engine.

We should also probably proactively check for the error case, ideally by having my hypothetical Context.synchronous_product_request transparently throw if there's an error.

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

There is already a helper method for this:

def product_request(self, product, subjects):
"""Executes a request for a single product for some subjects, and returns the products.
:param class product: A product type for the request.
:param list subjects: A list of subjects for the request.
:returns: A list of the requested products, with length match len(subjects).
"""
return self.products_request([product], subjects)[product]

So:

execute_process_result, = self.context._scheduler.product_request(ExecuteProcessResult, [req])

(note trailing comma to unpack the list)

This comment has been minimized.

@ity

ity Jan 10, 2018

Member

that cleaned it up- thanks! used product_request in test and forgot about it here.

@@ -53,20 +54,33 @@ def console_output(self, targets):

report_file = os.path.join(tmpdir, 'report_file')

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

Throw in a TODO that list_file, report_file and ignored_file should be relative files within the execution "chroot", and that list_file should be part of an input files Snapshot, and report_file and ignored_file part of an output files Snapshot, when we have that capability.

This comment has been minimized.

@ity

ity Jan 5, 2018

Member

done


# See http://cloc.sourceforge.net/#options for cloc cmd-line options.
cmd = (
self._get_cloc_script(),

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

Would be interesting to discuss how we migrate BinaryUtil to be more Snapshot-friendly, rather than passing around absolute paths, but definitely not a blocker here.

Might be worth adding a TODO, though.

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

I expect that it will involve adding an intrinsic to do the network fetch directly into a Snapshot.

This comment has been minimized.

@ity

ity Jan 10, 2018

Member

done

def process_request_from_java_sources(sources_snapshot, binary):
env = dict(os.environ)
return ExecuteProcessRequest(
args=binary.prefix_of_command() + tuple(['-version']) + tuple(f.path for f in sources_snapshot.files),

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

s/tuple(['-version'])/('-version',)/

def test_javac_version_example(self):
sources = PathGlobs.create('', include=['inputs/src/java/simple/Simple.java'])
scheduler = self.mk_scheduler_in_example_fs([
ExecuteProcess.create_in(product_type=ExecuteProcessRequest,

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

nit: wrap

root_entries = scheduler.execute(request).root_products
self.assertEquals(1, len(root_entries))
state = self.assertFirstEntryIsReturn(root_entries, scheduler)
concatted = state.value

self.assertEqual(Concatted('one\ntwo\n'), concatted)

def test_javac_version_example(self):
sources = PathGlobs.create('', include=['inputs/src/java/simple/Simple.java'])

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

Possibly not the right place to be asking but... Why do we need to use a glob here? Is it because it's the only way to turn a file into a Stat when handing it to the engine? It seems kind of a weird UX to be globbing to point at a single file (particularly as glob has the property where if the file is missing, that's silently ignored)...

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

Yes, currently the only way. A bit of sugar to create an "infallible" PathGlobs instance that handles erroring for a miss would work. But not critical now.

root_entries = scheduler.execute(request).root_products
self.assertEquals(1, len(root_entries))
state = self.assertFirstEntryIsReturn(root_entries, scheduler)
concatted = state.value

self.assertEqual(Concatted('one\ntwo\n'), concatted)

def test_javac_version_example(self):
sources = PathGlobs.create('', include=['inputs/src/java/simple/Simple.java'])

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

The actual command ignores the file here; is there a reason to pass one?

self.assertEquals(1, len(root_entries))
state = self.assertFirstEntryIsReturn(root_entries, scheduler)
result = state.value
self.assertEqual(0, result.exit_code)

This comment has been minimized.

@illicitonion

illicitonion Dec 21, 2017

Contributor

Assert that the stderr contains 'javac'?

@stuhood
Copy link
Member

stuhood left a comment

Thanks Ity/Daniel.

@@ -153,9 +164,105 @@ def create(product_type, binary_type, input_selectors, input_conversion, output_
# Return a task triple that executes the function to produce the product type.
return TaskRule(product_type, inputs, func)


This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

Will need two spaces to satisfy the linter (here and elsewhere in the file).

def create_snapshot_rules():
"""Intrinsically replaced on the rust side."""
return [
SingletonRule(_Snapshots, _Snapshots('/dev/null'))
]

def _execute_process(input_conversion,

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

This method is now unused, and should be removed.

use std::error::Error;
use std::collections::{BTreeMap, HashSet};

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

Rather than BTreeMap, which sorts its keys, you'll want to use OrderMap, which just maintains key ordering. It's already imported in this file.

This comment has been minimized.

@illicitonion

illicitonion Jan 4, 2018

Contributor

I'm pretty sure that this should be a BTreeMap; in the remote case, these are required to be sorted, and in the local case, having them be canonical for cache-key purposes is good.

(Incidentally, we should probably error here if a key is present twice :))

This comment has been minimized.

@stuhood

stuhood Jan 4, 2018

Member

I think that I had been thinking that order was relevant in ENV, but perhaps it is not. So yea, makes sense.

@@ -15,6 +15,8 @@ use context::Context;
use core::{Failure, Key, Noop, TypeConstraint, Value, Variants, throw};
use externs;
use fs::{self, Dir, File, FileContent, Link, PathGlobs, PathStat, VFS};
use handles::maybe_drain_handles;

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

Unused, afaict.

fn run(self, _: Context) -> NodeFuture<ProcessResult> {
let request = self.0.clone();
future::ok(ProcessResult(
process_executor::local::run_command_locally(request)

This comment has been minimized.

@stuhood

stuhood Dec 25, 2017

Member

We shouldn't... the explicit call in fn gen_nodes should be replaced with a request for this Node implementation.

I have a plan to refactor the python/rust interaction for process execution, so please don't spend any more time polishing the python/rust boundary than you need to to get the tests passing.

@ity

This comment has been minimized.

Copy link
Member

ity commented Jan 5, 2018

this took a backseat over holidays and other priorities - will pick it back up tonight/tomorrow

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looking good :) A few more things to clean up

result = state.value.exit_code
if result != 0:
# TODO: Longer term we need to figure out what to put on $PATH in a remote execution env.
# Currently, we are adding everything within $PATH to the request.

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

... Because the cloc script looks up perl in $PATH

"""
:param args: Arguments to the process being run.
:param env: A tuple of environment variables and values.

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

I quite liked it being a dict which we turn into a tuple; the fact that we don't have a way to pass dicts to the rust shouldn't be a concern of the calling API.

@@ -81,6 +80,18 @@ def _snapshotted_process(input_conversion,

return output_conversion(process_result, sandbox_dir)

def _setup_process_execution(input_conversion, snapshot_directory, *args):

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

Not done?

def create_snapshot_rules():
"""Intrinsically replaced on the rust side."""
return [
SingletonRule(_Snapshots, _Snapshots('/dev/null'))
]

def _execute_process(input_conversion,

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

TODO ^^

class ExecuteProcessResult(datatype('ExecuteProcessResult', ['stdout', 'stderr', 'exit_code'])):
pass

class Command(datatype('Command', [])):

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

Remove?

fn run(self, _: Context) -> NodeFuture<ProcessResult> {
let request = self.0.clone();
future::ok(ProcessResult(
process_executor::local::run_command_locally(request)

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

What's the plan here?

args: externs::project_multi_strs(&value, "args"),
env: env,
};
let result = process_executor::local::run_command_locally(request).unwrap();

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

TODO ^^

@@ -25,7 +25,7 @@ pub struct ExecuteProcessRequest {
///
/// No shell expansion will take place.
///
pub argv: Vec<String>,
pub args: Vec<String>,

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

TODO ^^

class ExecuteProcessResult(datatype('ExecuteProcessResult', ['stdout', 'stderr', 'exit_code'])):
pass

class Command(datatype('Command', [])):

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

TODO ^^

logger.debug('Running command: "{}" in {}'.format(command, sandbox_dir))
# TODO At some point, we may want to replace this blocking wait with a timed one that returns
# some kind of in progress state.
req = ExecuteProcessRequest(command, dict(os.environ))

This comment has been minimized.

@illicitonion

illicitonion Jan 10, 2018

Contributor

TODO ^^

@ity ity force-pushed the ity:cloc_rules_split_1 branch from d28592b to 39dd19c Jan 12, 2018

@@ -40,7 +40,7 @@ If unspecified, local execution will be performed.",
.get_matches();

let argv: Vec<String> = args
.values_of("argv")

This comment has been minimized.

@illicitonion

illicitonion Jan 16, 2018

Contributor

This should be argv (or the "argv" on line 37 needs to change)

The one on line 60 is the one which will cause CI to break

@@ -29,7 +29,7 @@ def _run_command(binary, sandbox_dir, process_request):
stderr=subprocess.PIPE,
stdout=subprocess.PIPE,
cwd=sandbox_dir)
# TODO At some point, we may want to replace this blocking wait with a timed one that returns

This comment has been minimized.

@stuhood

stuhood Jan 17, 2018

Member

This entire method should now be unused and deleted; if it isn't, it would mean some invoke had not been ported to the new codepaths.

This comment has been minimized.

@ity

ity Jan 17, 2018

Member

some of the tests (in test_isolated_process.py) have input/output file requirements that still use this method. I left this in, so that I wouldn't have to remove those tests. Once I have an equivalent set of tests (exercising input/output files) using the process execution in Rust, I will remove this and the corresponding tests.

argv: externs::project_multi_strs(&value, "argv"),
env: env,
};
// TODO: this should run off-thread, and asynchronously

This comment has been minimized.

@stuhood

stuhood Jan 17, 2018

Member

This should be requesting the Node that invokes the process, rather than doing it directly. Can leave as a TODO if you'd like.

@ity ity force-pushed the ity:cloc_rules_split_1 branch from 20cf86d to 726f0c7 Jan 19, 2018

@ity ity force-pushed the ity:cloc_rules_split_1 branch from 76a7cd7 to f00ce76 Jan 19, 2018

ity added some commits Jan 19, 2018

@ity ity merged commit 4d64273 into pantsbuild:master Jan 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stuhood added a commit that referenced this pull request Jan 24, 2018

Port IsolatedProcess implementation from Python to Rust - Split 1 (#5239
)

* Port IsolatedProcess implementation from Python to Rust - Part 1
Problem
From #4397 - Isolated processes currently work for the purposes of testing, but there are lots of abstraction leaks: in particular, a _Snapshots snapshots object is exposed which the python code uses to reverse engineer the location of snapshots.
Instead, process execution should be ported to rust (maybe using duct?) to remove that leak.
Additionally, the outcome of discussion around https://docs.google.com/document/d/1jGq34ds_fhRLPDnmHNV_FHcJiJnbhmk2XEwOf9-w5To/edit#heading=h.ylneg9wnwxcs was that we should adapt the process execution model (which is implicitly also a "remote process execution" model) more closely with the draft of bazel's process execution model.

Solution
This is the first split of the bigger branch (master...ity:ity/cloc_rules) that ports the implementation to Rust. This PR can go in independently and will make it easier to work with smaller branches.

Changes made in this PR:
Invoke process execution in Rust from Python
Plumbing for ExecuteProcessRequest/ExecuteProcessResult
cloc can be invoked from commandline which invokes Rust process execution 
Test (test_javac_version_example) added in python which invokes Rust through Rules.
Removed directories_to_create after discussing with @illicitonion
@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Feb 25, 2018

This broke cloc.py. For example, this line references result, which isn't defined in this side of the branch:

raise TaskError('{} ... exited non-zero ({}).'.format(' '.join(cmd), result))

Presumably the tests pass because that branch is not reached in practice, but this needs fixing.

Also, I'm not keen on a task reaching in to private state of Context (self.context._scheduler). Why is that necessary?

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Feb 25, 2018

Also, the call to ExecuteProcessRequest() appears to be missing arguments.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Feb 25, 2018

Also, I'm not keen on a task reaching in to private state of Context (self.context._scheduler). Why is that necessary?

It's supposed to be a temporary arrangement until #4769 is implemented.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Feb 26, 2018

Reverted in #5518 - will add tests before re-enabling

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