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

Skip pyprep and pytest-prep if there are no python targest #6039

Merged
merged 4 commits into from Jun 28, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jun 27, 2018

This saves 4.75s from a simple junit test run.

Fixes #5158

We could fix this in a more principled way by modifying how prepare works. If we assume that a BuildGraph is always constructed by the engine, and never mutated (or is only mutated up until a known point), we could pass a BuildGraph (or possibly an index of {TargetType: target_count}) into prepare, and not require products which won't actually be needed based on the BuildGraph. This probably isn't worth a lot of effort, because v2 should also solve this problem, but if we can easily make the no-mutation guarantee, may be worthwhile. There are a lot of these surprisingly-expensive landmines which occasionally add up to multiple seconds of unnecessary work.

Skip pyprep and pytest-prep if there are no python targest
This saves 4.75s from a simple junit test run.

I'm going to see if I can make the logic still execute, but more quickly,
but kicked this off to see how CI fares while I investigate.

@illicitonion illicitonion requested a review from baroquebobcat Jun 27, 2018

illicitonion added some commits Jun 27, 2018

@stuhood stuhood requested a review from cosmicexplorer Jun 27, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me.

I'd like to see a test that covers the empty case for select interpreters, but I'm not 100% sure whether it'd buy enough relative to it's cost.

I also have a minor wording request on the new TaskError.

@@ -95,7 +101,7 @@ def se(me, *args, **kwargs):
return interpreter.version_string

def test_interpreter_selection(self):
self.assertEquals('IronPython-2.77.777', self._select_interpreter([]))

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 27, 2018

Contributor

It might make sense to add a test that asserts that no interpreter is created when there are no targets. I don't feel super strongly about it though.

Maybe something like

     empty_target_roots=[]
     context = self.context(target_roots=empty_target_roots)
     task = self.create_task(context)

     # Mock out the interpreter cache setup, so we don't actually look for real interpreters
     # on the filesystem.
     with mock.patch.object(PythonInterpreterCache, 'setup', autospec=True) as mock_resolve:
       def se(me, *args, **kwargs):
         me._interpreters = self.fake_interpreters
         return self.fake_interpreters
       mock_resolve.side_effect = se
       task.execute()
 
    self.self.assertIsNone(context.products.get_data(PythonInterpreter))

This comment has been minimized.

@illicitonion

illicitonion Jun 28, 2018

Contributor

Done

# an empty set of sources, to prevent downstream tasks from having to check
# for this special case.
target_set_id = 'no_targets'
raise TaskError('Tried to get pex for no targets in gather_sources')

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 27, 2018

Contributor

I think the message here could be better, in case an execution does end up here. Maybe

Can't create pex in gather_sources: No relevant python targets provided

This comment has been minimized.

@illicitonion

illicitonion Jun 28, 2018

Contributor

Done

@benjyw
Copy link
Contributor

benjyw left a comment

To your question on slack, I am 100% fine with this.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jun 28, 2018

And ad-hoc solutions are fine IMO, I'd rather we put energy into porting tasks to the new engine, where none of this will be an issue anyway.

@illicitonion illicitonion merged commit 602445e into pantsbuild:master Jun 28, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/skippyprep branch Jun 28, 2018

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