pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.25k stars 625 forks source link

Pants plugin specs should be any pip-resolvable string #8669

Open benjyw opened 4 years ago

benjyw commented 4 years ago

Currently the plugin resolver assumes that a plugin is specified via a requirements string (e.g., my.cool.plugin==1.2.3). We should allow anything pip can consume (a URL, a VCS reference, and so on).

ns-iknox commented 4 years ago

Any plans to work on this @benjyw? I have a use case for this and would be willing to take a whack at it if you're not available for it.

benjyw commented 4 years ago

Hmm, I think the actual plugin resolution part might just work now, since everything is delegated to pex, which now delegates to pip: https://github.com/pantsbuild/pants/blob/b23771d0be272e2fb3e239d38c5a92c4d387c163/src/python/pants/init/plugin_resolver.py#L97

If I'm right about that then the remaining problem is just this line: https://github.com/pantsbuild/pants/blob/23a14d7ec45fb7bb38e89840df8bec86505e8ebb/src/python/pants/init/extension_loader.py#L108

And all that does that matters is getting hold of the project name for each requirement, so it can use it as a key to look up the dist in the pkg_resources.WorkingSet instance. But this can be done in a different way for URL/VCS requirements, as documented here: https://pip.pypa.io/en/stable/reference/pip_install/#working-out-the-name-and-version

E.g., it can come from the #egg=name suffix, or it can be parsed out of the filename for sdists/wheels.

Note that the WorkingSet.find() method expects a Requirement instance, and validates the semver, which we might not know in other cases, so we'd have to instead reach into the by_key dict directly, but that should be fine.

So if you're down to implement this, that would be great! Probably most of the work would just be in the testing!

benjyw commented 4 years ago

Adding @jsirois , who knows more about this than I do.

benjyw commented 4 years ago

I'm fine with requiring the #egg=name suffix for URLs in plugin specs and punting on parsing the filename, BTW.

ns-iknox commented 4 years ago

@benjyw Thanks for the background. We've ended up doing a semi-hacky workaround for now but I'll still try to take a quick swing at this later in the week.

stuhood commented 3 years ago

Adjusting how we accomplish plugin loading would be related to #9902.

jsirois commented 3 years ago

This issue really has nothing to do with loading plugins, just resolving them.

stuhood commented 3 years ago

This issue really has nothing to do with loading plugins, just resolving them.

I think that the potential relevance I was anticipating is that if in #9902 we move toward requiring that source plugins are built from real targets, that might be accomplished by building a wheel or otherwise isolated PYTHONPATH entry, and then loading from that (rather than placing un-isolated code directly on pants' PYTHONPATH, which causes the isolation/file-watching challenge from #9902).

jsirois commented 3 years ago

Aha - ok. Yeah, I had actually started down an experimental road supporting implicit output targets ala bazel for this. The idea would be plugins = ["//this/is/a/target.wheel"]. So, a target with an extension suffix where we'd have 2 new things, a function like ~this:

@union
class ImplicitInput:
  ...

def maybe_parse_implicit_output(spec: str) -> Optional[Tuple[str, Type[ImplicitInput]]:
  ...

And then a bit of code that used this like ~this in a @rule (although the plugin code would likely use the scheduler API to ask for the product directly?):

implict_output = maybe_parse_implicit_output(plugin)
if implicit_output:
  spec, implicit_input_type = implict_output
  plugin_distribution = await Get(ImplicitOutput, ImplicitInput, implicit_input_type(spec)) 

With plugins defining rules that accept their subclass of ImplicitInput (WheelRequest say) producing an ImplicitOutput file.

stuhood commented 3 years ago

If I understand correctly, then yea... that would likely work? But I'm not sure that it actually needs to be an abstract interface, because for the near term all plugins will be written and loaded as Python... and I think that that means that we could request a concrete type for the plugin.

Given that all plugins are Python, I think that you can use a concrete type rather than an abstract interface (ImplicitX). So maybe we'd have concrete @rules in the python backend to produce a concrete type PythonpathEntry (a wheel, directory, etc, in whatever form) for a target. And then to load source plugins, we'd have a "bootstrap Scheduler" instance with all binary plugins loaded (but no source plugins), and we'd request the PythonpathEntry for source plugin targets. In theory, that would allow us to use any binary plugin to bootstrap the source plugins (including dependency inference, etc). And because the loading would have occurred via a bootstrap Scheduler, we'd have file watching and etc to allow for (eventually) doing hot reloading.