Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More declarative target generators for plugins #14377

Merged
merged 7 commits into from
Feb 10, 2022

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Feb 6, 2022

#13882 will require adjusting how many instances of a target are created based on how many times the parametrize keyword appears in its fields (including in the overrides field, in the case of target generators). The parametrize keyword is not itself a valid field value (and would also be challenging to render as such in peek), and so target generation must be updated to consume something other than the instantiated target generator to decide which fields to generate with.

To prepare for these changes, the base ("template") and overridden fields to use in target generation are added to the GenerateTargetsRequest class, and made required in generate_file_level_targets. To reduce the total amount of churn in callers of generate_file_level_targets, this change introduces the TargetGenerator and TargetFilesGenerator base classes for Target. The latter allows file-target generation to be requested declaratively, and both allow specifying which fields should be "moved" and "copied" from the target generator to the generated targets. These moved/copied declarations are used to construct the template for generated targets.

[ci skip-rust]
[ci skip-build-wheels]

@stuhood stuhood force-pushed the stuhood/more-declarative-generators branch 11 times, most recently from 45a187c to 575e0af Compare February 7, 2022 22:39
@stuhood stuhood force-pushed the stuhood/more-declarative-generators branch 3 times, most recently from 6b0f46e to ab0b8aa Compare February 9, 2022 22:52
# Split out the `propagated_fields` before construction.
generator_fields = dict(target_adaptor.kwargs)
template_fields = {}
# TODO: Require for all instances before landing.
Copy link
Member Author

@stuhood stuhood Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The impact of not having done this is just that TargetGenerator subclasses (but not TargetFilesGenerator subclasses) are not able to consume the moved/copied fields, and are still consuming those fields from the generator class rather than the template. It's inconsistent, but I'd like to defer it to #14408 given the size of this PR.

src/python/pants/engine/target_test.py Show resolved Hide resolved
src/python/pants/help/help_info_extracter.py Show resolved Hide resolved
src/python/pants/init/engine_initializer.py Show resolved Hide resolved
@stuhood stuhood force-pushed the stuhood/more-declarative-generators branch from ab0b8aa to 4001102 Compare February 9, 2022 22:59
@stuhood stuhood marked this pull request as ready for review February 9, 2022 22:59
@stuhood
Copy link
Member Author

stuhood commented Feb 9, 2022

The commits are useful to review independently. The first is unfortunately a mishmash, because it was refactored so many times. But the second one is entirely updating file-generating targets to use the new API.

copied_fields = (PythonTestsDependenciesField,)
copied_fields = (
PythonTestsDependenciesField,
InterpreterConstraintsField,
Copy link
Member Author

@stuhood stuhood Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will fail (with a useful error message) to parametrize any core_field of a target generator, and so in order to parametrize interpreter_constraints, we'd need to make InterpreterConstraintsField a moved_field here. But there were tests (and maybe implementations?) relying on the current behavior of "all" targets in the graph having the field, so it felt best to tackle it in a followup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah imo this should be a moved_field. We should not be using the python_sources in the interpreter constraints calculation, that is a bug.

But agreed that it's best fixed in a followup.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving feedback on first commit. Will keep reviewing.

src/python/pants/engine/internals/graph.py Show resolved Hide resolved
src/python/pants/engine/internals/graph.py Outdated Show resolved Hide resolved
src/python/pants/build_graph/address.py Show resolved Hide resolved
src/python/pants/engine/target.py Outdated Show resolved Hide resolved
[ci skip-rust]
[ci skip-build-wheels]
[ci skip-rust]
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
[ci skip-rust]
[ci skip-build-wheels]
@stuhood stuhood force-pushed the stuhood/more-declarative-generators branch from e0ccb1c to 89bcdf1 Compare February 10, 2022 04:59
Copy link
Member Author

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have applied the review feedback, and split out some #14408-specific affordances to apply over there.

src/python/pants/engine/internals/graph.py Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing parametrization!

Comment on lines -125 to -126
# Note: Avro files cannot import from other Avro files, so do not add dependencies.
add_dependencies_on_all_siblings=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bummer we lose this comment. I think it's good to force plugin authors to reason about this, even though it's more boilerplate. What about requiring that you implement the Settings? Getting this option wrong is a big deal, it makes file-level targets irrelevant. And we messed it up in the past with files and resources.

(Fine if followup).

Copy link
Member Author

@stuhood stuhood Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that "dependencies on all siblings" should be rare to set, and unlike the inverse it's challenging to detect that you've set it accidentally (because dependencies are automatically more coarse-grained than they should be, and so things will succeed). The inverse (not setting it when you wanted to) is much, much easier to detect.

For example: scala has accidentally been setting this via add_dependencies_on_all_siblings, without us realizing (or remembering, at least): see #14382. In the interests of making dependency inference a stronger default, and removing the boilerplate required to do the right thing, I don't think that we should require it.

copied_fields = (
*COMMON_TARGET_FIELDS,
Dependencies,
JvmCompatibleResolvesField,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be moved? You move the singular resolve field, which makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, yes. Let's see if it encounters the same issue as InterpreterConstraintsField, if so, will defer. I'll be getting a PR out to replace this field later today I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah: yea, there are some consumers/tests expecting all targets in a cycle to have either a resolve or JvmCompatibleResolvesField. I'm going to back this out, and get another PR out for replacing this field.

copied_fields = (PythonTestsDependenciesField,)
copied_fields = (
PythonTestsDependenciesField,
InterpreterConstraintsField,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah imo this should be a moved_field. We should not be using the python_sources in the interpreter constraints calculation, that is a bug.

But agreed that it's best fixed in a followup.

src/python/pants/backend/scala/compile/scalac_test.py Outdated Show resolved Hide resolved
continue
elif field.value != field.default:
generated_target_fields[field.alias] = field.value
generated_target_fields = dict(template)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why call this template rather than directly generated_target_fields?

Copy link
Member Author

@stuhood stuhood Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking on this was that the generator might add additional fields... I didn't want to give the impression that it needed to be a complete set.

Comment on lines +879 to +880
# The base Address to generate for. Note that due to parametrization, this may not
# always be the Address of the underlying target.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would it be if not the underlying target?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of parametrization, the generator will have been constructed with its core_fields, and the moved_fields will have been parametrized in the template. The template and the template_address will then be aligned in terms of which parameters have been used, but the generator target cannot be: no choices were made about its fields, since any parametrized fields have been moved off.

@Eric-Arellano
Copy link
Contributor

Also this is a Plugin API change, not internal. At least one user had been using target gen for their plugin, although I think not anymore.

@kaos
Copy link
Member

kaos commented Feb 10, 2022

At least one user had been using target gen for their plugin, although I think not anymore.

I'm using target gen in a plugin internally at work :)

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the comments I could remember from #14408 so there are comments in there that may apply to this PR
#14408 (review)

src/python/pants/engine/target.py Outdated Show resolved Hide resolved
src/python/pants/engine/target.py Show resolved Hide resolved
@stuhood stuhood changed the title [internal] More declarative target generators More declarative target generators for plugins Feb 10, 2022
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
…y some code which expects it to exist on all targets.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood stuhood merged commit 6bbd9d8 into pantsbuild:main Feb 10, 2022
@stuhood stuhood deleted the stuhood/more-declarative-generators branch February 11, 2022 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants