A new Python run task. #4142

Merged
merged 2 commits into from Dec 20, 2016

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Dec 16, 2016

  • This is the first useful task in the new Python pipeline (the others
    are just preparation tasks).

  • Required small fixes to the interpreter selection task, namely
    consulting the correct constraints option, and remembering the
    interpreter's "extras" (as subsequent code needs to consult them).

  • Its integration test is identical to that of the old Python run task,
    with just option name changes, so that's some reasonable reassurance...

  • The new task is registered under the "run2" goal, so the old and new
    can co-exist. This will allow repo owners to try out the new pipeline
    in their repos without disrupting other users (especially since the
    old global "interpreters" flag is not consulted in the new pipeline,
    so some option setting on the new tasks may be necessary).

  • Note that the new pipeline does not yet support test/repl/bundling
    or codegen. Subsequent changes will provide this functionality,
    presumably pending some refactoring of the codegen backend.

A new Python run task.
- This is the first useful task in the new Python pipeline.

- Required small fixes to the interpreter selection task, namely
  consulting the correct constraints option, and remembering the
  interpreter's "extras" (as subsequent code needs to consult them).

- Its integration test is identical to that of the old Python run task,
  with just option name changes, so that's some reasonable reassurance...

- The new task is registered under the "run2" goal, so the old and new
  can co-exist.  This will allow repo owners to try out the new pipeline
  in their repos without disrupting other users (especially since the
  old global "interpreters" flag is not consulted in the new pipeline,
  so some option setting on the new tasks may be necessary).

- Note that the new pipeline does not yet support test/repl/bundling
  or codegen.  Subsequent changes will provide this functionality,
  presumably pending some refactoring of the codegen backend.

@benjyw benjyw requested review from jsirois and kwlzn Dec 16, 2016

+ if isinstance(binary, PythonBinary):
+ # We can't throw if binary isn't a PythonBinary, because perhaps we were called on a
+ # jvm_binary, in which case we have to no-op and let jvm_run do its thing.
+ # TODO(benjy): Some more elegant way to coordinate how tasks claim targets.

This comment has been minimized.

@stuhood

stuhood Dec 16, 2016

Member

This can be accomplished with MutexTask, which is used for a similar case with the repl tasks: https://github.com/pantsbuild/pants/blob/master/src/python/pants/task/mutex_task_mixin.py

@stuhood

stuhood Dec 16, 2016

Member

This can be accomplished with MutexTask, which is used for a similar case with the repl tasks: https://github.com/pantsbuild/pants/blob/master/src/python/pants/task/mutex_task_mixin.py

This comment has been minimized.

@benjyw

benjyw Dec 20, 2016

Contributor

Mentioned that in the TODO.

@benjyw

benjyw Dec 20, 2016

Contributor

Mentioned that in the TODO.

@kwlzn

kwlzn approved these changes Dec 19, 2016

lgtm

+ pexes = [self.context.products.get_data(ResolveRequirements.REQUIREMENTS_PEX),
+ self.context.products.get_data(GatherSources.PYTHON_SOURCES)]
+
+ pex_path = os.pathsep.join([pex.cmdline()[1] for pex in pexes])

This comment has been minimized.

@kwlzn

kwlzn Dec 19, 2016

Member

would be nice as a follow-up to expose a new property in pex for this vs relying on the pex.cmdline[1] access. maybe a TODO here?

@kwlzn

kwlzn Dec 19, 2016

Member

would be nice as a follow-up to expose a new property in pex for this vs relying on the pex.cmdline[1] access. maybe a TODO here?

This comment has been minimized.

@benjyw

benjyw Dec 20, 2016

Contributor

Added a TODO.

@benjyw

benjyw Dec 20, 2016

Contributor

Added a TODO.

+ for arg in self.get_options().args:
+ args.extend(safe_shlex_split(arg))
+ args += self.get_passthru_args()
+ po = pex.run(blocking=False, args=args, env={ 'PEX_PATH': pex_path })

This comment has been minimized.

@kwlzn

kwlzn Dec 19, 2016

Member

nit: ws around the env dict literal

@kwlzn

kwlzn Dec 19, 2016

Member

nit: ws around the env dict literal

This comment has been minimized.

@benjyw

benjyw Dec 20, 2016

Contributor

Removed.

@benjyw

benjyw Dec 20, 2016

Contributor

Removed.

@@ -50,7 +50,7 @@ def product_types(cls):
@classmethod
def register_options(cls, register):
super(SelectInterpreter, cls).register_options(register)
- # TODO: This will eventually supercede the global --interpreter flag. That flag is only
+ # TODO: This supercedes the global --interpreter flag. That flag is only

This comment has been minimized.

@kwlzn

kwlzn Dec 19, 2016

Member

should the TODO here be killed also?

@kwlzn

kwlzn Dec 19, 2016

Member

should the TODO here be killed also?

This comment has been minimized.

@benjyw

benjyw Dec 20, 2016

Contributor

Modified from a TODO to a note - I do want to emphasize this until we get rid of --interpreter.

@benjyw

benjyw Dec 20, 2016

Contributor

Modified from a TODO to a note - I do want to emphasize this until we get rid of --interpreter.

+ '--pyprep-interpreter-constraints=CPython>=2.6,<3',
+ '--pyprep-interpreter-constraints=CPython>=3.3',
+ '--quiet']
+ print('XXXXX ' + ' '.join(command))

This comment has been minimized.

@kwlzn

kwlzn Dec 19, 2016

Member

stray debug print?

@kwlzn

kwlzn Dec 19, 2016

Member

stray debug print?

This comment has been minimized.

@benjyw

benjyw Dec 20, 2016

Contributor

D'oh!

@benjyw

benjyw Dec 20, 2016

Contributor

D'oh!

@benjyw benjyw merged commit a585910 into pantsbuild:master Dec 20, 2016

1 check passed

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

@benjyw benjyw deleted the benjyw:python_run2 branch Dec 20, 2016

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

A new Python run task. (#4142)
- This is the first useful task in the new Python pipeline.

- Required small fixes to the interpreter selection task, namely
  consulting the correct constraints option, and remembering the
  interpreter's "extras" (as subsequent code needs to consult them).

- Its integration test is identical to that of the old Python run task,
  with just option name changes, so that's some reasonable reassurance...

- The new task is registered under the "run2" goal, so the old and new
  can co-exist.  This will allow repo owners to try out the new pipeline
  in their repos without disrupting other users (especially since the
  old global "interpreters" flag is not consulted in the new pipeline,
  so some option setting on the new tasks may be necessary).

- Note that the new pipeline does not yet support test/repl/bundling
  or codegen.  Subsequent changes will provide this functionality,
  presumably pending some refactoring of the codegen backend.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment