v2 repl-python goal #9077

gshuflin · 2020-02-06T01:24:46Z

This commit adds pants support for a v2 Python REPL goal, called repl2-python, which spawns a Python repl as a PEX build with the dependencies of the specified targets.

As per the discussion summarized in #9104, this goal is consistent with the plan of attack of implementing v2 repl support generally as a set of many goals, one per supported repl, that all know how to filter source files to only the language(s) they can interpret.

Eric-Arellano

Exciting!

Could you please add V2 integration tests, e.g. GoalRuleTestBase-style? python_test_runner_integration_test.py and isort/rules_integration_test.py might be good examples.

Eric-Arellano · 2020-02-06T16:30:14Z

src/python/pants/backend/python/rules/repl.py

+
+class ReplOptions(GoalSubsystem):
+  """Opens a REPL."""
+  name = 'repl2-python'


Medium term, I like the proposal to rename this to python. For now, this is a good name because it makes clear that this is still a WIP and subject to changes.

Eric-Arellano · 2020-02-06T16:31:08Z

src/python/pants/backend/python/rules/repl.py

+  exit_code = result.process_exit_code
+
+  if exit_code == 0:
+    console.write_stdout("REPL exited successfully")


Nit: probably good to use periods with both these messages.

Eric-Arellano · 2020-02-06T16:33:36Z

src/python/pants/backend/python/rules/repl.py

+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+import logging
+from pathlib import Path


Semi-nit: it looks like PurePath would work for everything in this file and I recommend changing to it. PurePath is a superclass of Path and has all the same methods as Path, except for methods that actually inspect the real filesystem. For example, there is no PurePath.exists() or PurePath.is_file().

Why use PurePath? It ensures we don't accidentally leak into the filesystem when we didn't intend to and it better expresses the intent of the code, that you're solely doing path manipulation and not actually reading from the filesystem.

Eric-Arellano · 2020-02-06T16:34:33Z

src/python/pants/backend/python/rules/repl.py

+    global_options: GlobalOptions) -> Repl:
+
+
+  hydrated_targets = await Get[HydratedTargets](BuildFileAddresses, build_file_addresses)


Instead, you can directly request HydratedTargets in the rule. It looks like you aren't using build_file_addresses anywhere else.

Also, I think we need TransitiveHydratedTargets?

gshuflin · 2020-02-11T02:35:54Z

Working on the integration test has revealed that there's room for improvement with the current test infrastructure for python tests. Once this PR is merged I want to refactor some of this code into a common Python v2 test class of some kind.

Eric-Arellano

Yay! Thank you for getting this working! Neat how little code it takes.

For the tests, what are your thoughts on somehow feeding stdin to the REPL to confirm that it's truly interactive? I think we discussed the stdin being something like calling exit(), which should cause the REPL to close right away. Add a timeout so that if the input doesn't work, we don't hang the test.

Eric-Arellano · 2020-02-11T02:38:10Z

src/python/pants/backend/python/rules/repl.py

+    build_root: BuildRoot,
+    global_options: GlobalOptions) -> PythonRepl:
+
+  # note - when eric's changes are merged, the .to_address() won't be necessary


Hopefully tomorrow! Close to getting this working..finishing last parts of getting V1 to still work, which still needs to use BuildFileAddress.

Eric-Arellano · 2020-02-11T02:39:05Z

src/python/pants/backend/python/rules/repl.py

+  create_pex = CreatePexFromTargetClosure(
+    addresses=python_addresses,
+    output_filename="python-repl.pex",
+    entry_point=None,


The default is None. I think it's fine to leave this off.

Eric-Arellano · 2020-02-11T02:39:55Z

src/python/pants/backend/python/rules/repl.py

+  repl_pex = await Get[Pex](CreatePexFromTargetClosure, create_pex)
+
+  with temporary_dir(root_dir=global_options.pants_workdir, cleanup=False) as tmpdir:
+    path_relative_to_build_root = str(PurePath(tmpdir).relative_to(build_root.path))


@codealchemy started using as_posix() instead. It does the same thing, minus not working on Windows, and I think is nicer to read:

Suggested change

path_relative_to_build_root = str(PurePath(tmpdir).relative_to(build_root.path))

path_relative_to_build_root = PurePath(tmpdir).relative_to(build_root.path).as_posix()

Eric-Arellano · 2020-02-11T02:40:14Z

src/python/pants/backend/python/rules/repl.py

+      DirectoryToMaterialize(repl_pex.directory_digest, path_prefix=path_relative_to_build_root)
+    )
+
+    full_path = str(PurePath(tmpdir, repl_pex.output_filename))


Ditto with using as_posix

Eric-Arellano · 2020-02-11T02:41:19Z

src/python/pants/backend/python/rules/repl.py

+  exit_code = result.process_exit_code
+
+  if exit_code == 0:
+    console.write_stdout("REPL exited successfully.")


I'm not sure if this is worth keeping, although it's fine in the short term. Logging exceptions seems helpful, but presumably it will be clear to the user once the REPL stops running.

I really like the idea of having a specific line of output with --v2-ui turned on, kind of like the current SUCCESS or FAILURE output, that signals the pants run completed? But that hopefully can be decoupled from the implementation of individual rules.

gshuflin · 2020-02-11T02:49:24Z

Yay! Thank you for getting this working! Neat how little code it takes.

For the tests, what are your thoughts on somehow feeding stdin to the REPL to confirm that it's truly interactive? I think we discussed the stdin being something like calling exit(), which should cause the REPL to close right away. Add a timeout so that if the input doesn't work, we don't hang the test.

Yeah, I think this would be a good thing to have, it will just require a little bit more work to build. I think I will try to get that chunk of work added to this PR tomorrow.

cosmicexplorer · 2020-02-11T22:18:54Z

src/python/pants/backend/python/rules/repl.py

+  exit_code = result.process_exit_code
+
+  if exit_code == 0:
+    console.write_stdout("REPL exited successfully.")


I really like the idea of having a specific line of output with --v2-ui turned on, kind of like the current SUCCESS or FAILURE output, that signals the pants run completed? But that hopefully can be decoupled from the implementation of individual rules.

cosmicexplorer · 2020-02-11T22:21:40Z

src/python/pants/backend/python/rules/repl_integration_test.py

+      download_pex_bin.DownloadedPexBin.Factory.global_instance(),
+    ]
+
+    output = self.execute_rule(args=["src/python:some_lib"], additional_params=additional_params)


Is it possible to mock input being sent to the repl? This was one of the biggest difficulties about testing the v1 repl. Is it possible to mock stdin being sent to interactive processes?

Not currently. #9108 is an issue I assigned myself to implement this work, but I don't think it's trivial to do.

as for the line of output, yeah, it would be nice if all v2 REPLs could use the same bit of code to output some "pants run complete" message. Whether or not that will be possible depends on whether we build some kind of new hierarchy level grouping all possible repl goals, cf. #9104

Oh, I was thinking rather that the v2 ui itself could output that sort of message for all goals, as opposed to v2 repls specifically.

Oh I see what you mean. That's not a bad idea actually, although it's not a change i want to try to make in this commit.

Is it possible to mock input being sent to the repl? This was one of the biggest difficulties about testing the v1 repl. Is it possible to mock stdin being sent to interactive processes?

Piping input to an integration test works fine (under pantsd too!): see

pants/tests/python/pants_test/backend/python/tasks/test_python_repl_integration.py

Lines 9 to 19 in 93c3567

@ensure_daemon

def test_run_repl(self):

# Run a repl on a library target. Avoid some known-to-choke-on interpreters.

command = ['repl',

'--python-setup-interpreter-constraints=CPython>=2.7,<3',

'testprojects/src/python/interpreter_selection:echo_interpreter_version_lib',

'--quiet']

program = 'from interpreter_selection.echo_interpreter_version import say_hello; say_hello()'

pants_run = self.run_pants(command=command, stdin_data=program)

output_lines = pants_run.stdout_data.rstrip().split('\n')

self.assertIn('echo_interpreter_version loaded successfully.', output_lines)

stuhood

Sorry for the delay! I replied to #9104: I think that there is a pretty straightforward way to avoid "goal per language", and it would be good to consider it before landing this.

gshuflin · 2020-02-13T07:19:45Z

Sorry for the delay! I replied to #9104: I think that there is a pretty straightforward way to avoid "goal per language", and it would be good to consider it before landing this.

No worries. I left some comments on that issue; I think there's still a bit of discussion to be had over exactly what v2 repl goals ought to look like. In the meantime, in the interest of getting this patch merged, would be okay to rename this goal from repl2-python to just repl2, and punt on the issues brought up there until the time comes to implement v2 support for repls besides the default python one?

stuhood · 2020-02-13T19:24:32Z

src/python/pants/backend/python/rules/repl.py

+  name = 'repl2-python'
+
+
+class PythonRepl(Goal):


I think that a reasonable amount of decoupling to begin heading in the direction of #9104 would be to install the repl goal in the core with a union, and to then fill the union in the python backend.

Eric-Arellano

Thanks! It's neat how little code it took to get this working.

Eric-Arellano · 2020-02-14T21:07:28Z

src/python/pants/rules/core/repl.py

+@union
+class ReplDeterminer:
+  pass


Maybe ReplImplementation?

So, to be clear, possibly in a followup this will be an option like ./pants repl2 --implementation=ipython? For now, we're punting on the specific option because there's only one possible implementation so it's unambigous? But this gives the infrastructure for more down the road?

Yes that's what I had in mind with this type.

stuhood

Thanks for putting in the time to generalize this! It looks like it needs a minor tweak to actually use the union: once it's green feel free to land.

stuhood · 2020-02-15T17:48:56Z

src/python/pants/rules/core/repl.py

+
+class ReplOptions(GoalSubsystem):
+  """Opens a REPL."""
+  name = 'repl2'


Does registration fail when this has the name repl? It feels like it shouldn't...

For example: we were able to install v2 test in the appropriate place... the only reason we couldn't for fmt/lint was that they had recursive options on them.

stuhood · 2020-02-15T18:01:48Z

src/python/pants/rules/core/repl.py

+
+
+@union
+class ReplImplementation:


Nothing is actually using this union, so this is likely to still cause "isolated backend" test failures.

I think that to actually bring the union into play, you'll need to construct an instance of the union member. That would look like taking UnionMembership, finding "a matching repl instance" (because this hasn't been fully generalized to inspect the target types, you could do something like "taking the first one"), constructing an instance of it (PythonRepl in this case), which will allow you to remove the targets_to_python_repl rule.

Constructing an instance of a union member might look like:

# TODO: We're only expecting one of these right now: to further generalize this, we'll want # to finish #4535. repl_impl = next(union_membership.union_rules[ReplImplementation]) repl_binary = await Get[ReplBinary](ReplImplementation, repl_impl(addresses))

gshuflin requested review from Eric-Arellano, pierrechevalier83 and cosmicexplorer February 6, 2020 01:25

Eric-Arellano reviewed Feb 6, 2020

View reviewed changes

gshuflin force-pushed the repl-python branch 2 times, most recently from fb12be7 to 7ccb304 Compare February 11, 2020 02:32

Eric-Arellano approved these changes Feb 11, 2020

View reviewed changes

This was referenced Feb 11, 2020

Create a mixin for V2 Python tests #9107

Closed

Create a mock InteractiveRunner #9108

Closed

gshuflin force-pushed the repl-python branch 4 times, most recently from 1ee1a65 to a59f988 Compare February 11, 2020 22:01

cosmicexplorer reviewed Feb 11, 2020

View reviewed changes

gshuflin force-pushed the repl-python branch 2 times, most recently from e17a5d7 to dfe94af Compare February 12, 2020 23:06

stuhood requested changes Feb 13, 2020

View reviewed changes

stuhood reviewed Feb 13, 2020

View reviewed changes

gshuflin force-pushed the repl-python branch 4 times, most recently from 7d95714 to 2c8ffa9 Compare February 14, 2020 21:00

Eric-Arellano approved these changes Feb 14, 2020

View reviewed changes

gshuflin force-pushed the repl-python branch 2 times, most recently from 5a1676d to 93514f9 Compare February 14, 2020 21:26

stuhood approved these changes Feb 15, 2020

View reviewed changes

gshuflin force-pushed the repl-python branch from 93514f9 to 09f46d3 Compare February 18, 2020 21:34

gshuflin added 10 commits February 18, 2020 14:25

Python repl

e119898

Respond to PR comments

00d740a

Rename Repl -> PythonRepl

b067bd3

Avoid BuildFileAddresses

66107be

Add integration test

ba79836

PR comments

f8e6662

Fix tests with new BinaryTool commit

b1de414

Fix name of import

f221db9

Convert to a top-level repl rule

63de9b0

Rename ReplDeterminer -> ReplImplementation, add docstring

701ec0b

gshuflin force-pushed the repl-python branch from 09f46d3 to fee2497 Compare February 18, 2020 22:25

Responses to PR comments

4d6b5f7

gshuflin force-pushed the repl-python branch from fee2497 to 4d6b5f7 Compare February 19, 2020 00:03

gshuflin merged commit e10b0d2 into pantsbuild:master Feb 19, 2020

gshuflin deleted the repl-python branch February 19, 2020 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2 repl-python goal #9077

v2 repl-python goal #9077

gshuflin commented Feb 6, 2020 •

edited

Loading

Eric-Arellano left a comment

Eric-Arellano Feb 6, 2020

Eric-Arellano Feb 6, 2020

Eric-Arellano Feb 6, 2020

Eric-Arellano Feb 6, 2020

Eric-Arellano Feb 6, 2020

gshuflin commented Feb 11, 2020

Eric-Arellano left a comment

Eric-Arellano Feb 11, 2020

Eric-Arellano Feb 11, 2020

Eric-Arellano Feb 11, 2020

Eric-Arellano Feb 11, 2020

Eric-Arellano Feb 11, 2020

cosmicexplorer Feb 11, 2020

gshuflin commented Feb 11, 2020

cosmicexplorer Feb 11, 2020

cosmicexplorer Feb 11, 2020

gshuflin Feb 11, 2020

gshuflin Feb 11, 2020

cosmicexplorer Feb 11, 2020

gshuflin Feb 11, 2020

stuhood Feb 13, 2020 •

edited

Loading

stuhood left a comment

gshuflin commented Feb 13, 2020 •

edited

Loading

stuhood Feb 13, 2020

Eric-Arellano left a comment

Eric-Arellano Feb 14, 2020

Eric-Arellano Feb 14, 2020

gshuflin Feb 14, 2020

stuhood left a comment

stuhood Feb 15, 2020

stuhood Feb 15, 2020 •

edited

Loading

		global_options: GlobalOptions) -> Repl:


		hydrated_targets = await Get[HydratedTargets](BuildFileAddresses, build_file_addresses)

	path_relative_to_build_root = str(PurePath(tmpdir).relative_to(build_root.path))
	path_relative_to_build_root = PurePath(tmpdir).relative_to(build_root.path).as_posix()

	@ensure_daemon
	def test_run_repl(self):
	# Run a repl on a library target. Avoid some known-to-choke-on interpreters.
	command = ['repl',
	'--python-setup-interpreter-constraints=CPython>=2.7,<3',
	'testprojects/src/python/interpreter_selection:echo_interpreter_version_lib',
	'--quiet']
	program = 'from interpreter_selection.echo_interpreter_version import say_hello; say_hello()'
	pants_run = self.run_pants(command=command, stdin_data=program)
	output_lines = pants_run.stdout_data.rstrip().split('\n')
	self.assertIn('echo_interpreter_version loaded successfully.', output_lines)

v2 repl-python goal #9077

v2 repl-python goal #9077

Conversation

gshuflin commented Feb 6, 2020 • edited Loading

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gshuflin commented Feb 11, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gshuflin commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

gshuflin commented Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Feb 15, 2020 • edited Loading

Choose a reason for hiding this comment

gshuflin commented Feb 6, 2020 •

edited

Loading

stuhood Feb 13, 2020 •

edited

Loading

gshuflin commented Feb 13, 2020 •

edited

Loading

stuhood Feb 15, 2020 •

edited

Loading