New python repl task. #4219

Merged
merged 3 commits into from Feb 10, 2017

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Jan 29, 2017

Uses the new python pipeline.

This involved a little refactoring, namely the creation of
two helper base classes:

  1. The PythonExecutionTaskBase base class provides the
    logic (ripped from the new PythonRun task) to "merge" pexes
    together using PEX_PATH, so you have a single pex to execute.

  2. The ResolveRequirementsTaskBase provides the logic for
    resolving requirements (ripped from the new
    ResolveRequirements task).

PythonExecutionTaskBase extends ResolveRequirementsTaskBase,
so it can resolve any extra requirements it needs in order to run
(e.g., ipython). This is a lightweight alternative to a full Python equivalent
of register_jvm_tool, as that would be significant work, and it's not yet
clear if that's a path we want to go down (esp. since the new engine should
provide an entirely different way of doing this sort of thing).

The old ResolveRequirements task also extends ResolveRequirementsTaskBase
of course, and the PythonRepl task introduced in this change extends
PythonExecutionTaskBase, as does the PythonRun task.

New python repl task.
Uses the new python pipeline.

This involved a little refactoring: The logic for resolving
requirements is now in a base class extended by the ResolveRequirements
task.  The PythonRepl task also extends it, so it can optionally
resolve iPython, if it needs to.

@benjyw benjyw requested review from kwlzn, jsirois and mateor Jan 29, 2017

@mateor

mateor approved these changes Jan 30, 2017

A bit of a rubber stamp, minus some nits. I am very interested in trying these in our repo but I have been struggling with Pex, specifically translation errors and leaks with the system python.

I would like to find a way to restrict the paths that are searched for an interpreter - that may be very easy and I just haven't discovered it yet. I will try this again in the next week or so.

@@ -0,0 +1,57 @@
+# coding=utf-8
+# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@mateor

mateor Jan 30, 2017

Member

I see headers from 2014, 2015 and 2016 :)

@mateor

mateor Jan 30, 2017

Member

I see headers from 2014, 2015 and 2016 :)

This comment has been minimized.

@benjyw

benjyw Jan 30, 2017

Contributor

Ha, yes, I shall fix the ones on the new files, anyway.

@benjyw

benjyw Jan 30, 2017

Contributor

Ha, yes, I shall fix the ones on the new files, anyway.

+ # TODO: should_build appears to be hardwired to always be True. Get rid of it?
+ if req.should_build(interpreter.python, Platform.current()):
+ reqs_to_build.add(req)
+ self.context.log.debug(' Dumping requirement: {}'.format(req))

This comment has been minimized.

@mateor

mateor Jan 30, 2017

Member

I considered that this white space in the log may be purposeful - but in that case the Skip message a couple lines down should probably match.

@mateor

mateor Jan 30, 2017

Member

I considered that this white space in the log may be purposeful - but in that case the Skip message a couple lines down should probably match.

This comment has been minimized.

@benjyw

benjyw Jan 30, 2017

Contributor

That was just copied from existing code, and I also think it is purposeful. I'll audit that this indenting makes sense.

@benjyw

benjyw Jan 30, 2017

Contributor

That was just copied from existing code, and I also think it is purposeful. I'll audit that this indenting makes sense.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Jan 30, 2017

Contributor

Re troubles with pexes - this new pipeline will make it easier to swap out pex. E.g., by using a virtualenv or something. But I haven't looked into this at all yet.

Contributor

benjyw commented Jan 30, 2017

Re troubles with pexes - this new pipeline will make it easier to swap out pex. E.g., by using a virtualenv or something. But I haven't looked into this at all yet.

@kwlzn

kwlzn approved these changes Jan 30, 2017

lgtm, handful of minor comments

+ Allows us to set the PEX_PATH in the environment when running.
+ """
+
+ def __init__(self, pex, extra_pex_paths, interpreter):

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

mind documenting the params here?

@kwlzn

kwlzn Jan 30, 2017

Member

mind documenting the params here?

This comment has been minimized.

@benjyw

benjyw Feb 2, 2017

Contributor

Done.

@benjyw

benjyw Feb 2, 2017

Contributor

Done.

+from pants.util.dirutil import safe_rmtree
+
+
+class PythonRepl(ReplTaskMixin, PythonExecutionTaskBase):

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

should this get a docstring for the task description?

@kwlzn

kwlzn Jan 30, 2017

Member

should this get a docstring for the task description?

This comment has been minimized.

@benjyw

benjyw Feb 2, 2017

Contributor

Done.

@benjyw

benjyw Feb 2, 2017

Contributor

Done.

+ return []
+
+ def setup_repl_session(self, targets):
+ tmpdir = tempfile.mkdtemp()

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

should this base the temp dir under .pants.d to keep lifecycle compartmentalized? or better yet, could this key to a reusable location under .pants.d?

@kwlzn

kwlzn Jan 30, 2017

Member

should this base the temp dir under .pants.d to keep lifecycle compartmentalized? or better yet, could this key to a reusable location under .pants.d?

This comment has been minimized.

@benjyw

benjyw Feb 2, 2017

Contributor

Yes, good idea. I completely neglected to do invalidation etc. I think I figured that this is always fast, but that's not true if resolving extra requirements, so I've done this properly now.

While doing so realized I can get rid of the custom fingerprint strategy for python requirements that I had - the normal strategy will do the right thing via the PythonRequirementLibrary's payload.

@benjyw

benjyw Feb 2, 2017

Contributor

Yes, good idea. I completely neglected to do invalidation etc. I think I figured that this is always fast, but that's not true if resolving extra requirements, so I've done this properly now.

While doing so realized I can get rid of the custom fingerprint strategy for python requirements that I had - the normal strategy will do the right thing via the PythonRequirementLibrary's payload.

+ def launch_repl(self, pex, **pex_run_kwargs):
+ po = pex.run(blocking=False, **pex_run_kwargs)
+ po.wait()
+ safe_rmtree(pex.path())

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

as-is, would it make sense to wrap the safe_rmtree here in a try/finally block to ensure execution in the case of execution failure?

@kwlzn

kwlzn Jan 30, 2017

Member

as-is, would it make sense to wrap the safe_rmtree here in a try/finally block to ensure execution in the case of execution failure?

This comment has been minimized.

@benjyw

benjyw Feb 2, 2017

Contributor

Got rid of this, so the pex can be reused.

@benjyw

benjyw Feb 2, 2017

Contributor

Got rid of this, so the pex can be reused.

from pants.util.contextutil import temporary_dir
from pants.util.strutil import safe_shlex_split
-class PythonRun(Task):
+class PythonRun(PythonExecutionTaskBase):

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

ditto re: docstring

@kwlzn

kwlzn Jan 30, 2017

Member

ditto re: docstring

This comment has been minimized.

@benjyw

benjyw Feb 2, 2017

Contributor

Done.

@benjyw

benjyw Feb 2, 2017

Contributor

Done.

+class ResolveRequirements(ResolveRequirementsTaskBase):

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

ditto re: docstring

@kwlzn

kwlzn Jan 30, 2017

Member

ditto re: docstring

This comment has been minimized.

@benjyw

benjyw Feb 2, 2017

Contributor

Done, although this isn't really intended to be run directly by humans.

We probably need some sort of distinction between goals that are user-facing and goals that aren't.

@benjyw

benjyw Feb 2, 2017

Contributor

Done, although this isn't really intended to be run directly by humans.

We probably need some sort of distinction between goals that are user-facing and goals that aren't.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Feb 9, 2017

Contributor

@kwlzn Could you take another look? I made non-trivial changes in response to your excellent comments.

Contributor

benjyw commented Feb 9, 2017

@kwlzn Could you take another look? I made non-trivial changes in response to your excellent comments.

@kwlzn

kwlzn approved these changes Feb 9, 2017

lgtm!

@benjyw benjyw merged commit 9b372d5 into pantsbuild:master Feb 10, 2017

1 check passed

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

@benjyw benjyw deleted the benjyw:python_repl branch Feb 10, 2017

peiyuwang pushed a commit to twitter/pants that referenced this pull request Feb 16, 2017

New python repl task. (#4219)
Uses the new python pipeline.

This involved a little refactoring: The logic for resolving
requirements is now in a base class extended by the ResolveRequirements
task.  The PythonRepl task also extends it, so it can optionally
resolve iPython, if it needs to.

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

New python repl task. (#4219)
Uses the new python pipeline.

This involved a little refactoring: The logic for resolving
requirements is now in a base class extended by the ResolveRequirements
task.  The PythonRepl task also extends it, so it can optionally
resolve iPython, if it needs to.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment