https://pantsbuild.org/ logo
Join Slack
Powered by
<@U051221NF>: i feel pretty strongly about <https:...
# development
w
@happy-kitchen-89482: i feel pretty strongly about https://github.com/pantsbuild/pants/pull/8985#issuecomment-577865695 ... i think you'll remember that we discussed this when the 5 toolchain folks visited
--owner-of
is popular, for good reason... it allows for the kind of syntax you're proposing
people want to be able to run the tests for a file, we know that. but how that is implemented for each test runner is very, very runner specific, and it's easy to imagine language test runners that wouldn't support it
and finally, i don't think that there are any implementation blockers for doing the thing i'm proposing. i think that it would probably make the patch smaller, if anything.
(...not sure about that. but if it doesn't, it's because there are potential pitfalls integrating owner-of with FilesystemSpecs... which is exactly the kind of thing we'd want to figure out sooner rather than later.)
cc @hundreds-father-404 ^
the "meshing of implementations" is less about v1 vs v2, and more about filesystemspecs vs owner-of
h
Hmm, there is still a pretty big distinction, in that 
--owner-of
implies that you're thinking in the world of targets, and filesystem specs imply that you're thinking in the world of files.
👍 1
The underlying source of disagreement here, I think, is about optimizing for the smooth transition of existing users, vs. the initial adoption curve, and overall experience, for new users. You're understandably focused on the former, we are very focused on the latter.
h
Shouldn’t make the diff smaller - the PR’s diff is almost entirely from introducing 
FilesystemSpec
, 
FilesystemSpecs
, and 
Specs
, then updating all the call sites to use the new names + updating 
cmd_line_spec_parser.py
to resolve between the two types of specs. We need those changes no matter what Also, we can always decide to deprecate 
--owner-of
in a followup that backports fs specs. While this PR adds messages saying fs specs don’t work with V1, those messages may be easily removed just as easily as they were added. I’m hoping to land the PR independent of the 
--owner-of
discussion so that it’s easier to work on followup PRs, like getting FS specs to emulate 
--owner-of
. Right now, FS specs only work with the toy examples of 
validate
and 
cloc
. But, this PR lays the foundation to do more powerful things
h
I may be misunderstanding something, but this change doesn't affect v1 at all I think? I.e., filesystem specs don't work in v1 and 
--owner-of
continues to work as usual, so this change only affects v2 users, and so seems like something we can continue to debate?
👍 1
w
the "world of files" does not exist except for in 
cloc
and 
validate
. i do not think it is worth biasing toward.
h
Or am I missing the intricacy of mixed v1/v2 mode? Is that the concern?
So I think in the eye of a user not already inclined towards thinking of targets, the "world of files" exists for everything. For them targets are the weird thing, not files...
w
i recognize that 
buildgen
is important as well, but you have yourself pointed out that targets should be secondary: so contorting to support a goal that shouldn't be necessary to run manually (because dep inference) isn't great
h
cloc
and 
validate
happen to not require targets at all in order to function, but there is a significant difference between "targets as background config" and "targets as the unit of CLI interaction"
👍 1
So for a user 
./pants test path/to/file_test.py
is very natural
w
no one is arguing for "targets as the unit of CLI interaction"
we are in violent agreement about that.
💯 1
h
Then I am confused
w
you can implement very little without targets
h
Not necessarily true with dep inference
w
meaning: 
--owner-of
and FilesystemSpecs are intimately related
h
But think of targets as config
w
@happy-kitchen-89482: dependency inference would still infer the target given a file
h
OK so that's what I'm not sure about. 
--owner-of
says "there is this thing called a target, that is the unit of CLI interaction, we happen to have a way of mapping files back to targets, but your operations are happening at the target level", whereas filesystem specs say "operations happen at the file level, they may get some config from these things called targets"
That feels very different to me
w
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1579822484021600?thread_ts=1579821109.019400&amp;cid=C0D7TNJHL my point is that FilesystemSpecs should deprecate 
--owner-of
. if they don't, then we are asking for confusion and pain.
h
Right, but in v1 I don't see a way to do that with the same semantics as v2 (i.e., that the operations are at file level)
👍 1
--owner-of
is not that
w
i described it on the ticket.
h
operations are still at the target level
w
they always will be.
as you said: the target is the config
h
OK that's what we're disagreeing on I think
w
you cannot run the test without config
if you did, then you are still settling on a "default" config
h
Right, but you can run it just on the file you were asked to run it on, not the other files that happen to share the same config
👍 1
And I think that's generally true
w
yep. but that is just an implementation detail.
i described exactly how to have the runner filter.
think of it this way:
the difference we're talking about is between goals exploding if they do not* have explicit support for operating on files baked in
...and having filesystem specs work for all goals that only support targets, while being able to also step in and layer in the more specific behaviour that a test runner would to filter down to one file
because most test runners can (not even all).
and certainly not all goals will be able to operate on files.
in those cases, you would blow up on the command line: "sorry, you gave me a file! i only understand targets"
h
That's the part I'm not sure I agree with.
What's an example?
w
any target or goal for which there is required config.
binary, bundle, etc.
publish
h
in those cases, you would blow up on the command line: “sorry, you gave me a file! i only understand targets”
In this case, the fs spec would act like 
--owner-of
and map back to the target
h
Wait wait
w
./pants publish path/to/some/file.py
# "Exception: this is a goal that can only understand targets"
h
It's not "sorry you gave me a file I only understand targets", it's "I need some config in order to operate on this file"
w
h
I know that seems like a small distinction, but it's conceptually large
w
@happy-kitchen-89482: in terms of the wording of the error message...?
i'm suggesting that there should not be an error message.
because it's not necessary.
h
In terms of the expected behavior. For example 
./pants binary path/to/file_with_an_entrypoint.py
is pretty unambiguous
So we should build a binary for that thing, there may not need to be a 
python_binary
target for it at all.
w
i don't think so.
h
It might be embedded in a 
python_library
We can get its deps from there
w
and how would a goal actually do that?
h
I mean I'm fine deprecating 
--owner-of
in favor of filesystem specs if there's a way to do that neatly in v1 (and it sounds like you're saying there is), but there will still be a discontinuity between how that behaves in v1 and v2, or at least how I'm envisioning v2 working... 🙂
w
by having two implementations, depending on whether it got a file or a target?
@happy-kitchen-89482: i don't think we're disagreeing about how it works from a user perspective.
h
We do seem to be going around in circles, which can be a sign of heated agreement...
So what, if anything, are we disagreeing on?
w
i'm saying that if someone did ``./pants binary path/to/file_with_an_entrypoint.py`` and you wanted that to work, you would either 1) have two implementations of the goal or special handling for files vs targets, 2) infer (possibly by generating it in memory) the relevant target to operate on
the latter (2) is 
--owner-of
.
h
I mean, wouldn't we just need to map a file to its owning target if such a target exists
The target can be an optional place for config related to the file (say, python interpreter constraints, dependencies, whatever)
w
if you wanted to work without such a target existing, you would still need to infer a target.
h
But the user interaction would always be files
usually, anyway
w
so perhaps a useful question is: what would you like to happen in the 
binary $file
case if there was a target owning the file?
you can't ignore the target, i think. because that is how someone specified the config
h
"take relevant config from that target, if it exists, use sensible defaults otherwise"
right, I'm just saying that it need not exist
w
ok. that's 
--owner-of
then.
--owner-of
plus dependency inference.
h
--owner-of
requires a target to exist, no?
w
in v1 it does, yea.
(currently)
@happy-kitchen-89482: again, the argument is that "a target always exists"
if it doesn't, you infer one... and voila, it exists
h
OK, this is in v2 target land where targets really are just strongly-typed config?
That I can wrap my brain around
But the difference remains when a target has multiple files in it
so maybe 
binary
is not the best example to use
w
my feeling is that if 
binary $file
, you really have to attempt to find the owning target on disk first
otherwise you'd ignore any config that was specified nearby in favor of ... doing the most literal thing? even if its the wrong thing
finally: about 
validate
, 
buildgen
, 
cloc
: they should be the special cases (in my mind) that don't even ask for addresses, and just directly consume filespecs
👍 1
h
sure, but the user did not mean "act on this target", they meant "act on this file". For example, what if the entrypoint file is just one of many files in a 
python_library
? All the useful config is on that library target, and the user is directly providing the entrypoint, which is usually the only salient thing a 
python_binary
provides that a 
python_library
does not. So a 
python_binary
need not exist at all, not even by inference.
w
@happy-kitchen-89482: that is "the most literal thing", but it is probably the wrong thing.
h
How so?
h
finally: about validate , buildgen , cloc : they should be the special cases (in my mind) that don’t even ask for addresses, and just directly consume filespecs
Ftr, that’s what I’m proposing in the design doc. I could see us eventually going to this future where targets don’t exist and/or are much less relevant, but for now every goal but 
validate
, 
buildgen
, and 
cloc
needs to map back to an owning target. The edge case is goals that should map back to owning targets + preserve the files specified to be more precise in how they operate, like 
test
and 
lint
. This is handled by the 
TargetWithSources
type in the proposal
👍 1
w
@happy-kitchen-89482: i don't want you to ignore my 
python_tests
config if i type 
./pants test $file
it's no less necessary because i used syntax sugar
h
no, but we can do the right thing if it doesn't exist.
w
@hundreds-father-404: ok. if that's the case, then deprecating owner-of should be straightforward.
@happy-kitchen-89482: yep, violent agreement there.
h
if that’s the case, then deprecating owner-of should be straightforward.
if we were okay with 
./pants test foo.py
meaning in V2 to try to only run over 
foo.py
(but still grabbing metadata from the owning target), whereas in V1 it always no matter what means the owning target. I find the difference in those semantics very confusing, personally, but am willing to trust others’ judgment on this (Originally, the other edge case I was concerned about is that there’s no way to port 
SourcesSnapshot
, i.e. get V1 goals to work on files that have no owners. I realized that’s irrelevant. Only 
cloc
, 
validate
, and 
buildgen
behave this way, and they already are implemented in V2)
w
but still grabbing metadata from the owning target
i think that you have to. see two or three above.
h
My point is that deprecating v1 --owner-of in favor of filesystem specs in v1 means that filesystem specs will have, in general, different semantics in v1 vs. v2. Some things will fail in v1 but succeed in v2 (because we can be sloppier about missing targets), and e.g., 
./pants test path/to/foo_test.py
will run just the tests in that file in v2, but all the tests in that file's owning target in v1. So it sounds like you're comfortable with those differences?
w
--owner-of
is not v1 only, it works with both v1 and v2
this has very little to do with v1 and v2
So it sounds like you're comfortable with those differences?
yes.
h
> but still grabbing metadata from the owning target
i think that you have to. see two or three above.
Right. I’m not debating resolving the owning target at all. The conversation took a turn towards that for a bit, but I view that part of the conservation as “this might be a possible future where we aren’t as wedded to targets.” For now, I think that’s the wrong discussion
w
k
(even in a world where we're "not wedded to targets", IMO it's because we generating a default in memory, not because we don't go looking for your config/BUILD if it exists)
👍 2
h
We always use your target if it exists
We are in heated agreement on that
w
ok, then we can deprecate 
--owner-of
with this mechanism.
h
> So it sounds like you’re comfortable with those differences?
yes.
This is the heart of the disagreement in my eyes, then. If we were okay with the different semantics between V1 vs. V2 goals—where V2 implementations were more powerful to attempt to only operate on the specified files whereas V1 is more imprecise and operates over teh whole owning target—then, yes, I agree with deprecating 
--owner-of
h
With the caveat of some differences in behavior between v1 and v2 when using filespecs (which would not be different with 
--owner-of
)
👍 1
w
right
h
OK, so it sounds like we basically agree after all
As long as there is a sensible way to implement filesystem specs for v1, which it sounds like there is. That part I have not looked into at all.
Sensible and not many weeks of work... 🙂
w
i do think that the " 
./pants $goal $file
always attempting to actually find the owning target before it decides to generate one" bit is an important takeaway from this discussion
h
Hm I do now see some concerns why 
./pants fmt2 foo.py
works but 
./pants fmt foo.py
fails. I think you’re right, Stu, that that confusion is even worse than the confusion of why 
./pants fmt2 foo.py
only runs on 
foo.py
whereas 
./pants fmt foo.py
runs over the whole target
always attempting to actually find the owning target before it decides to generate one” bit is an important takeaway from this discussion
*For any goal which does not request 
SourcesSnapshot
/ needs target info
w
*For any goal which does not request 
SourcesSnapshot
which is 
cloc
/ 
buildgen
/ 
validate
, right?
👍 1
er. those are the ones that do request 
SourcesSnapshot
👍 1
h
Yes, I think we all agree that if there is a target that owns a file, that target provides config for the goal. In v1 that if is always true. In v2 we can be more forgiving, but naturally if you provide explicit target info we use that, same as today.
w
k
h
As long as there is a sensible way to implement filesystem specs for v1, which it sounds like there is. That part I have not looked into at all.
Yep. My next task was getting fs specs to work like 
--owner-of
h
We can be more forgiving in the future, anyway
👍 1
w
yep.
thanks folks 😃
😁 1
h
@witty-crayon-22786 I’m hoping to land https://github.com/pantsbuild/pants/pull/8985 even though it currently does error if you try to use FS specs with V1, because it lays the foundation for that next feature. Notably, the PR will also error if you try to use FS specs with any goal other than 
cloc2
and 
validate
We’d remove those error messages as soon as that lands. But, for now, the error message is technically correct
h
To be super-duper clear, when I say "no targets" to me that's just shorthand for "targets are not (or at least need not) be the unit of CLI interaction", not "targets shouldn't exist"... And it sounds like we mostly agree on this.
👍 1
w
Yep
@hundreds-father-404: fine with that. Will review shortly.
h
I’ll update the PR description to explain the plan of deprecating 
--owner-of
. Will put up the RunTracker change in a second too
w
thanks much
shipit + comment
❤️ 1
thanks
❤️ 1
Open in Slack
PreviousNext
Powered by