[WIP] create a 'target_adaptors' entry in v2 plugins and backends #9153

cosmicexplorer · 2020-02-19T09:10:52Z

Problem

We would like to be able to consume v2 lint and fmt rules with Black in the Twitter monorepo. Right now this fails because we have many different target names for our python code. I believe this issue is something that will occur for many other repos attempting to switch to using Black with pants as well.

Solution

Allow v2 plugins to declare a def target_adaptors() function returning a dict mapping BUILD file symbols to TargetAdaptor subclasses.

TODO

Expose the mapping of TargetAdaptorWithOrigin in structs.py to register.pys in backends and plugins, the same way we've exposed target_adaptors().

Possibly use the target_adaptors() endpoint directly for this information?
See:

pants/src/python/pants/engine/legacy/structs.py

Lines 321 to 322 in 4e981bc

    
           def create(adaptor: TargetAdaptor, origin: OriginSpec) -> "TargetAdaptorWithOrigin": 
        
             adaptor_with_origin_cls: Type["TargetAdaptorWithOrigin"] = {

Result

Repos which use many different names for targets (e.g. python3_binary(), hadoop_binary(), etc) are now able to use rules acting on HydratedTargets in the v2 engine!

pierrechevalier83

Thanks for the change Danny. I can understand the motivation for it and it looks good from where I'm standing.

I would value a second opinion from someone who knows more about the architecture of the codebase to know if this is the "proper" way to add this functionality (configurable target_adaptors for python).

pierrechevalier83 · 2020-02-19T13:43:41Z

src/python/pants/build_graph/build_configuration.py

@@ -180,6 +185,10 @@ def union_rules(self):
    """
    return self._union_rules

+  def target_adaptors(self):
+    """???"""


I don't understand this doc comment.

That's a TODO mark. I had intended to remove it before posting :)

pierrechevalier83 · 2020-02-19T13:46:10Z

src/python/pants/build_graph/build_configuration.py

@@ -166,6 +167,10 @@ def register_rules(self, rules):
                              if rule.dependency_optionables}
    self.register_optionables(dependency_optionables)

+  def register_target_adaptors(self, adaptors):
+    # TODO: type annotations and runtime validation?


Did you mean to address this TODO before merging? (the type annotations at least sounds like something that can be done without much effort)

Yes! Thank you!

pierrechevalier83 · 2020-02-19T13:52:43Z

src/python/pants/init/engine_initializer.py

  # TODO: The alias replacement here is to avoid elevating "TargetAdaptors" into the public
  # API until after https://github.com/pantsbuild/pants/issues/3560 has been completed.
  # These should likely move onto Target subclasses as the engine gets deeper into beta
  # territory.
-  table['python_library'] = PythonTargetAdaptor


So I can see these moved to register.py and they get loaded in extension_loader.py, but I don't really understand the rationale behind this code move. Could you explain? (possibly in the commit message of a distinct commit to help review both changes independently)

This code move is the entire rationale for this PR. Right now, the only way to make a BUILD file target name map to a specific TargetAdaptor subclass is to put it in engine_initializer.py. Outside of the pants codebase (i.e. in the Twitter codebase), we would like to be able to e.g. run Black on our python targets, but right now since only the BUILD file string python_library is mapped to PythonTargetAdaptor, pants in v2 is unable to see any targets in the Twitter monorepo. After this change, the Twitter monorepo could expose 'python3_library': PythonTargetAdaptor in their repo's pants-plugins/, and we would be able to run Black on those targets!

blorente

Thanks! Being able to register adaptors in the plugins is great, much like we could to in v1 :)

The implementation looks good, but someone with more experience than me writing v2 rules should take a look.

blorente · 2020-02-19T17:28:38Z

src/python/pants/init/engine_initializer.py

@@ -132,17 +132,14 @@ def _legacy_symbol_table(build_file_aliases: BuildFileAliases) -> SymbolTable:
        tuple(factory.target_types)[0],
      )

+  table.update(target_adaptors)
+
  # TODO: The alias replacement here is to avoid elevating "TargetAdaptors" into the public


This comment can probably go away now 🎉

benjyw · 2020-02-19T21:50:06Z

Thanks for the change Danny! We have many issues with the lack of a proper target API, and this addresses a big one. I'm not totally clear on how this actually solves the problem though?

More generally, I think the whole target adaptor concept is intended to be a "temporary" hack, until we have a proper, stable target API? I'm not sure how I feel about codifying this hack in the plugin spec TBH. But @stuhood has more context here, would love to get his opinion.

Eric-Arellano · 2020-02-19T21:56:24Z

Agreed with Benjy, thank you for putting this up! I too am not certain if we should land this yet, though, because Stu and I will be finally implementing the Target API in early March on my next trip to SF. I believe our goal is to stop using TargetAdaptor, so I'm not sure we want to land this. Although, I am empathetic to the fact that this solves a real problem faced by Twitter. So, I'll defer to @stuhood

Another consideration worth pointing out is that this won't work when goals use TargetAdaptorWithOrigin, which currently is fmt2, lint2, and test. Those must be explicitly created as new classes and registered here:

https://github.com/pantsbuild/pants/blob/master/src/python/pants/engine/legacy/structs.py#L321-L338

This means that Twitter's target python3_binary would not work with fmt2 for example, because the Python3BinaryTargetAdaptorWithOrigin would not be registered so the above code would default to using TargetAdaptorWithOrigin.

cosmicexplorer · 2020-02-20T07:42:29Z

I'm not totally clear on how this actually solves the problem though?

Right now we cannot run lint2 or fmt2 in the Twitter repo because our python targets are not named python_library() due to a temporary migration scheme to python 3 that we are doing. The only blocker to doing this is making pants accept that our target names (e.g. python3_library()) map to the appropriate PythonTargetAdaptor. This lets the v2 engine "see" our code (otherwise it just no-ops).

More generally, I think the whole target adaptor concept is intended to be a "temporary" hack, until we have a proper, stable target API?

That is exactly what @stuhood has said. I also separately investigated in #8760 an approach which would allow us to make use of v1 targets directly as long as they don't try to access the build graph (which is just set to None). Since this PR allows us to actually use the v2 engine in the Twitter repo and we've allowed engine_initializer.py to stabilize somewhat, I think there may be reason to chalk this one up to "anything in the v2 API is still fair game to change or remove at any time". I could be wrong about that, but it feels cheap to add a register.py entrypoint right now, when there aren't a plethora of separate entities relying on it.

Stu and I will be finally implementing the Target API in early March on my next trip to SF.

Awesome! I have two responses to this:
(1) As usual, can I be involved? Do you have time for me to discuss the approach of #8760 as a partial answer?
(2) I would love to have some way to use the python v2 backend in our repo now, and this particular change (allowing the setting of TargetAdaptors to BUILD file symbols) also allows us to start prototyping things in v2 in our repo, such as e.g. Black support with 2-space indentation.

This is not intended to be an answer to #4535 whatsoever. This PR is intended to solve a specific problem: we cannot use the python v2 backend in our repo yet, because we can't map names to TargetAdaptor subclasses. We do not currently have the capability to build on top of the python backend in our repo because we do not have control of this BUILD file name mapping outside of engine_initializer.py, and Twitter does not fork pants internally, so we are blocked without this change.

Another consideration worth pointing out is that this won't work when goals use TargetAdaptorWithOrigin

This is now a TODO for this PR! Thank you so much!

Eric-Arellano

Right now we cannot run lint2 or fmt2 in the Twitter repo because our python targets are not named python_library() due to a temporary migration scheme to python 3 that we are doing.

This is very helpful context. Thank you for sharing that - this makes sense.

Given that this solves an immediate problem for Twitter now and that it possibly helps to constrain the Target API design, such that we need to ensure we support custom target types, I think at a high level it makes sense to land this once the below concerns are resolved.

--

Another consideration worth pointing out is that this won't work when goals use TargetAdaptorWithOrigin

This is now a TODO for this PR! Thank you so much!

Given that your goal is to be able to run fmt2 and lint2 internally, you must solve this problem to be able to use both those goals at Twitter. fmt2 and lint2 now both use TargetAdaptorWithOrigin et al., so it's not enough to register a specific TargetAdaptor. You must also define the corresponding TargetAdaptorWithOrigin and add it to the dictionary in legacy/structs mapping TargetAdaptor -> TargetAdaptorWithOrigin.

I think your proposal to read from the register.py endpoint would work. There are two things you must do:

Create a new frozen dataclass called Python3TargetAdaptorWithOrigin, which subclasses TargetAdaptorWithOrigin and has the field adaptor: Python3TargetAdaptor. This is weird - in general, runtime creation of classes can be hard to follow and I'm not sure if it plays nicely with dataclasses (legit question, I don't know).
Update the dictionary in TargetAdaptorWithOrigin.create() to map Python3TargetAdaptor to Python3TargetAdaptorWithOrigin.

--

Even then, fmt2 and lint2 still won't work out of the box because Python3TargetAdaptorWithOrigin would not be registered with the unions defined in python_format_target.py and python_lint_target.py:

pants/src/python/pants/backend/python/lint/python_lint_target.py

Lines 45 to 89 in d690ab2

    
           PYTHON_TARGET_TYPES = [ 
        
             PythonAppAdaptorWithOrigin, 
        
             PythonBinaryAdaptorWithOrigin, 
        
             PythonTargetAdaptorWithOrigin, 
        
             PythonTestsAdaptorWithOrigin, 
        
             PantsPluginAdaptorWithOrigin, 
        
           ] 
        
           @rule 
        
           def target_adaptor(adaptor_with_origin: PythonTargetAdaptorWithOrigin) -> _ConcretePythonLintTarget: 
        
             return _ConcretePythonLintTarget(adaptor_with_origin) 
        
           @rule 
        
           def app_adaptor(adaptor_with_origin: PythonAppAdaptorWithOrigin) -> _ConcretePythonLintTarget: 
        
             return _ConcretePythonLintTarget(adaptor_with_origin) 
        
           @rule 
        
           def binary_adaptor(adaptor_with_origin: PythonBinaryAdaptorWithOrigin) -> _ConcretePythonLintTarget: 
        
             return _ConcretePythonLintTarget(adaptor_with_origin) 
        
           @rule 
        
           def tests_adaptor(adaptor_with_origin: PythonTestsAdaptorWithOrigin) -> _ConcretePythonLintTarget: 
        
             return _ConcretePythonLintTarget(adaptor_with_origin) 
        
           @rule 
        
           def plugin_adaptor(adaptor_with_origin: PantsPluginAdaptorWithOrigin) -> _ConcretePythonLintTarget: 
        
             return _ConcretePythonLintTarget(adaptor_with_origin) 
        
           def rules(): 
        
             return [ 
        
               lint_python_target, 
        
               target_adaptor, 
        
               app_adaptor, 
        
               binary_adaptor, 
        
               tests_adaptor, 
        
               plugin_adaptor, 
        
               *(RootRule(target_type) for target_type in PYTHON_TARGET_TYPES), 
        
               *(UnionRule(LintTarget, target_type) for target_type in PYTHON_TARGET_TYPES) 
        
             ]

I believe you could get around this, though, by adding to your own Pants plugin a rule that converts Python3TargetAdaptorWithOrigin to _ConcretePythonLintTarget and also a UnionRule(LintTarget, Python3TargetAdaptorWithOrigin).

--

Finally, I don't think the custom targets will work how you'd like out of the box, e.g. a python3_library won't automatically apply interpreter constraints. I think you would solve that by adding a property called compatibility that always returns Python 3 to the Python3TargetAdaptor definition? Kinda like this:

pants/src/python/pants/engine/legacy/structs.py

Lines 209 to 220 in d690ab2

    
           class AppAdaptor(TargetAdaptor): 
        
             def __init__(self, bundles=None, **kwargs): 
        
               """ 
        
               :param list bundles: A list of `BundleAdaptor` objects 
        
               """ 
        
               super().__init__(**kwargs) 
        
               self.bundles = bundles 
        
             @addressable_list(Exactly(BundleAdaptor)) 
        
             def bundles(self): 
        
               """The BundleAdaptors for this JvmApp.""" 
        
               return self.bundles

(Granted, you might not care about this at the moment).

--

Before merging, can you please confirm that everything works as expected when running Pants from sources in the Twitter repo, particularly running on a python3_library?

Eric-Arellano · 2020-02-21T01:16:32Z

src/python/pants/backend/python/register.py

@@ -92,6 +99,16 @@ def register_goals():
  task(name='unpack-wheels', action=UnpackWheels).install()


+def target_adaptors():


Given that this is temporary, how would you feel about renaming this to _target_adaptors?

It's not strictly necessary because V2 is unstable so we could remove this without needing a deprecation, but register.py is generally very very public so the leading underscore might make it more clear this is experimental.

Eric-Arellano · 2020-02-21T01:18:31Z

src/python/pants/build_graph/build_configuration.py

@@ -180,6 +185,10 @@ def union_rules(self):
    """
    return self._union_rules

+  def target_adaptors(self):


Nit: should this be a property? I'm not sure why the others like rules() aren't.

Eric-Arellano · 2020-02-21T01:20:48Z

src/python/pants/backend/python/register.py

+  return {
+    'python_library': PythonTargetAdaptor,
+    'python_app': PythonAppAdaptor,
+    'python_tests': PythonTestsAdaptor,
+    'python_binary': PythonBinaryAdaptor,
+    'python_requirement_library': PythonRequirementLibraryAdaptor,
+  }


I think you'll want to add this to each of the register.pys for each of the V2 linters. We specifically set those up so that they work without having pants.backend.python registered.

--

Actually, how do you feel about moving these back into engine_initializer.py? I think that we always want these registered, at least for the time being before we have the Target API.

I think the intent here was cleanup, but if, instead, it was to test that this new mechanism works, consider adding a dedicated test.

Eric-Arellano · 2020-05-18T02:57:59Z

The entry point for the target API is now target_types :)

create adaptor tables in v2 plugins and backends

3772084

cosmicexplorer requested review from stuhood, jsirois, benjyw, pierrechevalier83, blorente, Eric-Arellano and gshuflin February 19, 2020 09:10

pierrechevalier83 reviewed Feb 19, 2020

View reviewed changes

blorente approved these changes Feb 19, 2020

View reviewed changes

cosmicexplorer changed the title ~~create a 'target_adaptors' entry in v2 plugins and backends~~ [WIP] create a 'target_adaptors' entry in v2 plugins and backends Feb 20, 2020

Eric-Arellano reviewed Feb 21, 2020

View reviewed changes

cosmicexplorer mentioned this pull request Feb 27, 2020

add type-checked get_field() method to TargetAdaptor and consume v1 target payloads #9188

Closed

Eric-Arellano closed this May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] create a 'target_adaptors' entry in v2 plugins and backends #9153

[WIP] create a 'target_adaptors' entry in v2 plugins and backends #9153

cosmicexplorer commented Feb 19, 2020 •

edited

Loading

pierrechevalier83 left a comment

pierrechevalier83 Feb 19, 2020

cosmicexplorer Feb 20, 2020

pierrechevalier83 Feb 19, 2020

cosmicexplorer Feb 20, 2020 •

edited

Loading

pierrechevalier83 Feb 19, 2020

cosmicexplorer Feb 20, 2020 •

edited

Loading

blorente left a comment

blorente Feb 19, 2020

cosmicexplorer Feb 20, 2020

benjyw commented Feb 19, 2020

Eric-Arellano commented Feb 19, 2020

cosmicexplorer commented Feb 20, 2020

Eric-Arellano left a comment

Eric-Arellano Feb 21, 2020

Eric-Arellano Feb 21, 2020

Eric-Arellano Feb 21, 2020

Eric-Arellano commented May 18, 2020

	def create(adaptor: TargetAdaptor, origin: OriginSpec) -> "TargetAdaptorWithOrigin":
	adaptor_with_origin_cls: Type["TargetAdaptorWithOrigin"] = {

	PYTHON_TARGET_TYPES = [
	PythonAppAdaptorWithOrigin,
	PythonBinaryAdaptorWithOrigin,
	PythonTargetAdaptorWithOrigin,
	PythonTestsAdaptorWithOrigin,
	PantsPluginAdaptorWithOrigin,
	]


	@rule
	def target_adaptor(adaptor_with_origin: PythonTargetAdaptorWithOrigin) -> _ConcretePythonLintTarget:
	return _ConcretePythonLintTarget(adaptor_with_origin)


	@rule
	def app_adaptor(adaptor_with_origin: PythonAppAdaptorWithOrigin) -> _ConcretePythonLintTarget:
	return _ConcretePythonLintTarget(adaptor_with_origin)


	@rule
	def binary_adaptor(adaptor_with_origin: PythonBinaryAdaptorWithOrigin) -> _ConcretePythonLintTarget:
	return _ConcretePythonLintTarget(adaptor_with_origin)


	@rule
	def tests_adaptor(adaptor_with_origin: PythonTestsAdaptorWithOrigin) -> _ConcretePythonLintTarget:
	return _ConcretePythonLintTarget(adaptor_with_origin)


	@rule
	def plugin_adaptor(adaptor_with_origin: PantsPluginAdaptorWithOrigin) -> _ConcretePythonLintTarget:
	return _ConcretePythonLintTarget(adaptor_with_origin)


	def rules():
	return [
	lint_python_target,
	target_adaptor,
	app_adaptor,
	binary_adaptor,
	tests_adaptor,
	plugin_adaptor,
	*(RootRule(target_type) for target_type in PYTHON_TARGET_TYPES),
	*(UnionRule(LintTarget, target_type) for target_type in PYTHON_TARGET_TYPES)
	]

	class AppAdaptor(TargetAdaptor):
	def __init__(self, bundles=None, **kwargs):
	"""
	:param list bundles: A list of `BundleAdaptor` objects
	"""
	super().__init__(**kwargs)
	self.bundles = bundles

	@addressable_list(Exactly(BundleAdaptor))
	def bundles(self):
	"""The BundleAdaptors for this JvmApp."""
	return self.bundles

		@@ -92,6 +99,16 @@ def register_goals():
		task(name='unpack-wheels', action=UnpackWheels).install()


		def target_adaptors():

[WIP] create a 'target_adaptors' entry in v2 plugins and backends #9153

[WIP] create a 'target_adaptors' entry in v2 plugins and backends #9153

Conversation

cosmicexplorer commented Feb 19, 2020 • edited Loading

Problem

Solution

TODO

Result

pierrechevalier83 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

cosmicexplorer Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

blorente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Feb 19, 2020

Eric-Arellano commented Feb 19, 2020

cosmicexplorer commented Feb 20, 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

Eric-Arellano commented May 18, 2020

cosmicexplorer commented Feb 19, 2020 •

edited

Loading

cosmicexplorer Feb 20, 2020 •

edited

Loading

cosmicexplorer Feb 20, 2020 •

edited

Loading