Deprecate awslambda and binary goals in favor of new package goal
#10881
Conversation
[ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
requested review from
stuhood,
jsirois,
benjyw and
cosmicexplorer
|
FYI @asherf |
|
Ignoring the implementation for now, I'm in favor of this in general: it's redundant to have a separate target type ( But I would like to hear from others (e.g., @stuhood , @asherf, @jsirois ) what they think. Perhaps it makes sense to post this to Slack as well and get some feedback. I also wouldn't mind doing a tiny bit of bikeshedding on the goal name. |
This is worth overthinking. This is an extremely public API that we won't be able to change again. |
|
How will a user specify that they want each of the following, in a JVM world?
|
Thoughts on the Goal Name
Although I've used Proposal for an Enum Option
This was a super helpful glossary, thanks! To me (especially in the way you've described them here), I would perhaps expect the type of output to be controlled by an enum option e.g. I tried to consider whether Example Usages
Alternatively:
Alternatively:
Alternatively: Separately, I assume you raised this use case because you think it's a good idea to have a separate option for generating such an unpacked directory, but since we already assume the user's machine has an
Alternatively: For the goals in the OP:
Since @illicitonion discussed the JVM binary approaches, what's left is python binaries.
Although, thinking over it again, I would also actually expect "creating something you can publish to an artifact repository" to be a separate goal, in particular I would assume we would run
Not too sure about this one as I haven't used it, but possibly: |
|
@cosmicexplorer Regarding the As a more specific example, we have a python library we use internally, but also publish as the entrypoint to a docker container (using a custom python_library(
...
)
docker_image(
...
)When we run our |
|
@jperkelens just dropped a great tidbit over slack:
This is a great argument to separate |
Yes, this is exactly what would happen. I don't think we can use a flag like Instead, we can avoid inventing any new mechanisms and leverage -- To answer @illicitonion's question:
jvm_binary(name="jar", dependencies=[...])
A new archive(
assets=["//:python_binary"],
files=["//:files1"],
format="zip",
)
To clarify, Your |
|
What I'm reading from @Eric-Arellano's answers is that if there are multiple "flavours" of things you may want in the If so, I think that On the other hand, this seems like a bit of a step away from minimal BUILD files. I was under the impression we were generally trying to get as close to not needing BUILD files (and indeed explicit build targets) as possible. If that is the direction we're trying to head, I would have expected us to lean more into goals (e.g. by invoking |
The alternative would be to instead have a
I don't understand what you mean by coerce. I explicitly said that a
Separately, it is absolutely possible to separate the sources and the dependencies of a
The pants v1 jvm backend supports multiple different output formats for a single input format. This is currently chosen between with goals. As stated above:
@illicitonion was not describing
Why are we relying on goals to call other goals? I was under the impression the v2 engine was intended to avoid that requirement?
Why aren't we using |
Yes, this is accurate.
The vision is to reduce boilerplate in BUILD files, such that all that remains is concise, declarative metadata about your code. We don't necessarily need to delete BUILD files entirely; they serve an important purpose, which is declaring metadata that Pants cannot infer from your code. Here, I think it would fit the bill of being "concise, declarative metadata" in saying in your BUILD file what the format would be, e.g.
One downside (imo) of instead expressing this metadata through the command line is that it's not checked into your repository; you have to remember to use the right combinations of flags every time you run Pants. This is an actual issue right now with how we implemented the Another issue is that it makes it much harder to run But, one clarification. We can still allow you to use options to influence things, if we want. For example, we could add an --
I think Pants likely will want to have a publish goal in the future, but we don't have it yet. We're trying to make this breaking API change before 2.0 lands to avoid changing things on new users. It's not in anyone's scope that I know of to implement Likewise, I think JP offered some interesting insight into their org's desire to have distinct
Yes, I was being slightly imprecise to keep the conversation higher level. The pants/src/python/pants/backend/python/goals/pytest_runner.py Lines 155 to 174 in efef4c5 That is, find all the Right now, this code only works with something that can be built via
If/when we add |
I don't think metadata is the same thing as the variance in output formats that was described for v1 jvm binary targets. The purpose of targets is absolutely to hold metadata. The proposed options or separate goals are for things that are independent of that metadata, such as the format you want the result to be in. Additionally, BUILD file target definitions should be considered as stable as command-line goals until #9434 is productionized. You also did not address this at all:
This is precisely the distinction I am trying to draw between metadata and output formats. Is your goal to just not support multiple output formats because you think it's unnecessary? Could you clarify that? Because I do not understand how your proposal would support the multiple output formats that were discussed above. The difference is not between
This invocation would also build tons of pex files across the repo, and you quote a user who specifically describes the importance of having those goals separated. Could you speak to how your proposed solution would address that? I'm incredibly confused by the below:
You have misunderstood me. I was not suddenly changing my proposal to say we should use Separately, in relation to the above, you directly refer to a user who specifically said they wanted different goals for I'm also incredibly confused by the below:
If we do want to then implement a
Why would we take shortcuts now, then, instead of trying to figure out a more general API that e.g. will work on jvm targets?
The conversation is about which goals to create to satisfy requirements. I clearly do not understand the level of abstraction you are looking for right now. With the warning that this is an API that we won't be able to change again, why are we looking to be imprecise?
I do not understand why this is the case and it seems to be a central justification for your proposal. |
|
This is probably not the right forum for this level of design discussion: it's challenging to respond directly to individual points without threading. But I would ask that everyone please recognize that the format is not ideal for discussion, and continue to assume that everyone has the same goal: making excellent software. I've suggested that @Eric-Arellano move the design discussion onto a document. |
I cannot tell you how much my heart sank to read this sentence. In the future when we're talking about decisions that "cannot be changed later" and are attached to an active PR please do not mistake a desire to contribute to a discussion for aggression and please make concerns about healthy discussion explicit (I do not know what this quote is referring to) and raise them in the appropriate forum. |
@cosmicexplorer : I will happily discuss that in a DM if you are interested. (EDIT: or in #committers if you would prefer that) |
Thanks for the suggestion, JP! # 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]
There was a problem hiding this comment.
@benjyw was extremely patient in describing to me the relevance of this PR in the short term, and noted especially that the act of merging the python_awslambda() and python_binary() targets (proposed in #10893) would be a much bigger change than I had previously realized. I will be thinking about that separately and asynchronously.
After thinking a lot about it and getting more familiar with the implementation along with extremely helpful context from @Eric-Arellano, I will push any further longer-term abstract ideas about how to improve the BUILD file API for plugin authors into docs and out of the way of active work. The change in this diff (merging awslambda and binary into a single build goal) is 100% reasonable and I deeply appreciate the patience demonstrated in explaining it to me so many times.
Thanks to @stuhood as well for stepping in and recognizing the miscommunication here. That was extremely helpful.
src/python/pants/core/goals/build.py
Outdated
|
|
||
|
|
||
| class BuildSubsystem(GoalSubsystem): | ||
| """Build an asset, such as an archive, JAR, PEX, AWS Lambda, etc.""" |
There was a problem hiding this comment.
Ah, this is the "asset" definition I was looking for! Thanks!
As mentioned in #10893, it's already possible to have ./pants help show information on "what's buildable" by using UnionRules (it's magic!!!!), so that addresses any other concerns I had for now (will think more on whether we can display it better).
There was a problem hiding this comment.
Let me put up a change to rename this all to package :) I really like JP and your feedback on that rename, rather than build.
src/python/pants/core/goals/build.py
Outdated
|
|
||
| @union | ||
| class BuildFieldSet(FieldSet, metaclass=ABCMeta): | ||
| """The fields necessary to build an asset from a target.""" |
There was a problem hiding this comment.
It feels not quite right to me to document this as "the fields necessary to build", when there are no fields declared and it's abstract. Perhaps
| """The fields necessary to build an asset from a target.""" | |
| """The `@union` necessary to hook up a `./pants build` implementation from a target""" |
There was a problem hiding this comment.
Check out the docs on how we present FieldSets to plugin authors: https://www.pantsbuild.org/docs/rules-api-and-target-api#fieldsets
While the superclass is an interface, this description is accurate for subclasses. In our plugin docs and the example plugin docs, we only show how you subclass a field set. We don't talk about creating the interface itself, as that's only relevant when you're creating a generic goal like test, lint, etc; at that advanced of a use case, we'd either expect users to read our code for inspiration, or to reach out to us for help.
There was a problem hiding this comment.
I personally would still then say in the docstring
| """The fields necessary to build an asset from a target.""" | |
| """An abstract base class for the fields necessary to build an asset from a target.""" |
because it otherwise looks like code was moved without docstrings being updated. But I would also accept what is there now, because
While the superclass is an interface, this description is accurate for subclasses
Separately, we don't currently enforce e.g. that if a @union class has metaclass=ABCMeta, that all of its implementors must subclass it. Would that be a useful check to add? Right now, I believe it's possible to add UnionRule(BuildFieldSet, NoneType) without causing any "compile-time" error, for example. I believe the intent of this pattern has now become pretty canonical.
There was a problem hiding this comment.
Separately, we don't currently enforce e.g. that if a @union class has metaclass=ABCMeta, that all of its implementors must subclass it. Would that be a useful check to add? Right now, I believe it's possible to add UnionRule(BuildFieldSet, NoneType) without causing any "compile-time" error, for example. I believe the intent of this pattern has now become pretty canonical.
Yes, I think this would be useful. There are now several times where we use this pattern of the superclass being an ABC, and expecting subclasses to implement it.
If possible, it'd be great for an implementation to leverage the stdlib and ABCs. I believe there are mechanisms already in Python to check if a subclass inherits an ABC (it might be as simple as isinstance).
Possible implementation:
base_is_an_interface = abc.is_abstract(union_base) # I didn't check if this existed
invalid_members = [member for member in union_members if not issubclass(member, base)]
if invalid_members:
raise Exception()I do not think we need to support having this check be used if you are not using an ABC for your union base. It does not need to be more general than the above, imo.
Also, it's important that we continue to support a union base not being an ABC and the members not being subclasses. Namely, how we implement PluginField, where there is no inheritance relationship.
# 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]
Eric-Arellano
changed the title
awslambda and binary goals in favor of new build goalawslambda and binary goals in favor of new package goal
We now have several goals that build an asset:
binaryawslambdasetup-pySoon, we will be adding support for an
archive()target type as part of #10733. Anarchiveis not really abinary, nor any of the other two. We don't want to add a fourth goalarchive. It gets confusing when there are lots of goals; and it also makes things likeruntime_binary_dependenciesless flexible, e.g. not working withpython_awslambda.Semantically, the
packagegoal is for transforming your code into a single exportable asset that goes into--distdir. This is different than, say, acompilegoal; running on a library target should no-op.This only deprecates
awslambdaandbinaryfor now;setup-pywill be a followup, as that doesn't port as easily due to passthrough args.See https://docs.google.com/document/d/1q77XvcIU9dCzuzA2iFRQQ9AGIfV9jpR269415HdEt0E/edit# for a design doc diving more into this.
[ci skip-rust]
[ci skip-build-wheels]