Write virtualenvs for python tools as part of export

[danxmoran]

Closes #14837

Still TODO:

• Add an integration test
• Add export hook to pytest plugin
• Figure out why existing test is timing out with these changes

[ci skip-rust]

[ci skip-build-wheels]

[Eric-Arellano]

Exciting!

Do you know where to go from here? I'm thinking that we could try adding Doc formatter as another hardcoded tool. That will make it a little more obvious how we can de-duplicate things.

Once we have that good to go, we can start to tackle the issue of interpreter constraints like flake8.

[Eric-Arellano]

Great progress so far!

After looking at what you have, I am pretty sure that we are going to need a dedicated plug-in hook. I was hoping that we can leverage the plug-in hook we already have for generating lock files. But I do not think that is sufficient because it does not have information about how to install from a lock file that already exists. It only tells us how to generate a new lock file.

As briefly discussed over direct message, it is unfortunate to require plug-in authors to implement a new plug-in implementation if they want to integrate with the export goal. But it is a worthy trade-off to have more boilerplate for a much better user experience, given how requested this feature is.

We will also try to make the boiler plate as minimal as possible.

--

To create a new plug-in hook, we are going to use unions: https://www.pantsbuild.org/docs/rules-api-unions

Define a class like ExportPythonToolSentinel. Empty for now, but add @union. This is our inspiration:

pants/src/python/pants/core/goals/generate_lockfiles.py

Lines 56 to 68 in cc4e180

	@union
	class GenerateToolLockfileSentinel:
	"""Tools use this as an entry point to say how to generate their tool lockfile.
	Each language ecosystem should set up a union member of `GenerateLockfile`, like
	`GeneratePythonLockfile`, as explained in that class's docstring.
	Then, each tool should subclass `GenerateToolLockfileSentinel` and set up a rule that goes from the
	subclass -> the language's lockfile request, e.g. BlackLockfileSentinel ->
	GeneratePythonLockfile. Register a union rule for the `GenerateToolLockfileSentinel` subclass.
	"""
	resolve_name: ClassVar[str]

pants/src/python/pants/backend/python/lint/isort/subsystem.py

Lines 97 to 105 in cc4e180

	class IsortLockfileSentinel(GenerateToolLockfileSentinel):
	resolve_name = Isort.options_scope
	@rule
	def setup_isort_lockfile(
	_: IsortLockfileSentinel, isort: Isort, python_setup: PythonSetup
	) -> GeneratePythonLockfile:
	return GeneratePythonLockfile.from_tool(isort, use_pex=python_setup.generate_lockfiles_with_pex)

Similar to the above, we want each rule for each tool to generally only be one line. Note that there are only two things that are different in your isort and Doc formatter rules:

isort_pex = await Get(Pex, PexRequest, isort.to_pex_request())
dest = os.path.join("python", "virtualenvs", "tools", isort.name)

So, we want our plug-in hook for each tool to return back to us a PexRequest and the name of the tool. You will want to create a new data class that can store both of those things. So each rule will look something like ExportIsortSentinel -> ExportPythonTool. Note that each rule does not need to actually use the Get(Pex) part. We only need the PexRequest. They each need to also register a UnionRule.

From there, we can define a rule that goes from ExportPythonTool -> ExportResult. This is basically the existing rule that you already have for both isort and Doc formatter, only made generic.

Finally, we need to call the new plug-in hook. In your rule export_virtualenvs, request the type union membership. Inspect it for ExportPythonToolSentinel. Use Get(ExportResult, ExportPythonToolSentinel, instance) in a comprehension, like this code:

pants/src/python/pants/core/goals/generate_lockfiles.py

Lines 351 to 354 in cc4e180

	specified_tool_requests = await MultiGet(
	Get(WrappedGenerateLockfile, GenerateToolLockfileSentinel, sentinel())
	for sentinel in requested_tool_sentinels
	)

(that will do some really cool engine magic. You'll go from ExportIsortSentinel -> ExportPythonTool -> ExportResult, without having to explicitly state the middle step.)

[Eric-Arellano]

Huzzah! That looks great! So now you will want to set that plug-in hook up for each python tool we have. You can search for GeneratePythonLockfile - put the new rules next to those ones.

In direct message, you mentioned an import cycle. You will need to delete the hardcoded iSort and doc formatter code from the file you have been using.

I wonder if we actually want to export every tool? There are some, like the Docker parser, which do not seem useful for end-users. What do you think?

--

Most tools will be exactly like you already have. The only tricky things I think will be ones that set interpreter constraints from user code, along w/ plugins:

pants/src/python/pants/backend/python/lint/flake8/rules.py

Lines 56 to 59 in cc4e180

	flake8.to_pex_request(
	interpreter_constraints=partition.interpreter_constraints,
	extra_requirements=first_party_plugins.requirement_strings,
	),

In that case, you can leverage what we already have with generating lock files. Use a line like Get(GeneratePythonLockfile, Flake8LockfileSentinel()). Then, use the interpreter constraints from that type. For the plug-in requirements, simply put Flake8FirstPartyPlugins in the rule signature

[danxmoran]

Deleted code here was moved to another file, see the new export_test.py

[stuhood]

Pushed two commits: the latter one actually addresses the issue.

[Eric-Arellano]

Awesome! This looks ready to merge to me beyond minor review feedback. Thank you 🎉

[Eric-Arellano]

Suggested change

	"""Pythion tools use this as an entry point to say how to export their tool virtualenv."""
	"""Python tools use this as an entry point to say how to export their tool virtualenv."""

[Eric-Arellano]

It would probably be good to add instructions in a new paragraph. Subclass this class then create a rule going from the sub class to ExportPythonTool. Register the union rule. If the tool is in pantsbuild/pants, update the export test.

[Eric-Arellano]

Excellent comment!

[Eric-Arellano]

It would be good to subclass EngineAwareParameter and implement debug_hint() to return self.resolve_name

[Eric-Arellano]

This variable could probably use a rename. Maybe all export tool requests

[Eric-Arellano]

Consider using an enum

[Eric-Arellano]

Or, calling it backend prefix might be better. Use none to represent built in

[Eric-Arellano]

This comment could probably be removed. Or, maybe reword it to say: "Note: not every tool implements --version so this is the best we can get."

[Eric-Arellano]

Nit: unnecessary white space imo since these lines are highly related. There are a couple other places in the PR like this. Feel free to ignore, not a very big deal

[Eric-Arellano]

Awesome! Thank you so much :) this will go out in the newest dev release for 2.12