https://pantsbuild.org/ logo
Join Slack
Powered by
<https://github.com/pantsbuild/pants/pull/8450> <@...
# development
a
https://github.com/pantsbuild/pants/pull/8450 @hundreds-father-404 this PR will go green again in a bit, wanted to mention that upon merging this we might be able to kill 
list_targets_old.py
entirely?
w
between this and 
query
, my feeling is that query is probably a higher priority. i’m concerned that our existing goals are limited with regard to things like hydration and fields, and i think that a query API might provide a better foundation to build upon.
đź‘Ť 1
my fundamental concern moving forward is that the concept of “dependencies” is very limited, and i’m worried about proceeding to quickly on ports of v1 goals without having more of this in place
the primary thing is that inference and codegen allow your dependencies to change based on how you are viewed
a
if your concern is about 
dependencies
, then we can remove that field from the json
i've been trying to merge this after being asked by multiple users many times to get this in, over multiple objections. i'll do the work to make it mergeable, as usual, but this is definitely useful
w
@aloof-angle-91616: the issue is just that the fingerprint is very likely to be wrong, and i don’t know how we’ll make it right.
a
would you like to justify that? the fingerprint comes from the exact 
_key()
method that we use to calculate the hash
w
which is gone in v2 =/
đź‘Ť 1
a
???
w
because how fingerprints are calculated is via process executions
a
i'm referring to the v2 
Struct
_key()
method
w
right, that’s not the fingerprint of the target
a
then why does it exist?
w
(you’re right: it’s not gone. but it’s not used as the fingerprint)
a
what do you mean by fingerprint?
w
process executions are how you hit the cache, anything else is in memory
a
yes, that is the goal of the fingerprint
w
so we don’t use the python structures as a “fingerprint” in a strong hashing sense: only as in-memory memoization
but in general: “what i depend on” and “what my fingerprint is” are 100% rule dependent in v2
a
this feature is one users have specifically asked for, and the current implementation solves the issues. i had to push a very hacky v1 console task inside of twitter last week because this wasn't merged yet
w
it’s not a question of “what is the fingerprint of a target”, it’s “what is the fingerprint of the target when used in this rule/process execution”
a
if you're going to block features that users actually want on "the output doesn't conform entirely to my personal platonic ideal of what a 'fingerprint' is", which is something you're extremely fond of doing, i'm not going to waste time fighting you again and again
w
if we land something that we can’t maintain moving forward, we anger users in a different way
or that breaks on them
i’m trying to explain why this is likely to be hard to maintain, or to break
đź‘Ť 1
a
i'm pretty sure that this solution just invalidates too coarsely as opposed to not invalidating correctly. are you able to describe why that's not the case?
also, this is build graph information that by definition is separate from any v2 rule. this is intended to be consumable by external tools -- those external tools by definition are not running v2 rules.
i really think some thought on how this is supposed to be used would be useful here.
w
so, i think that is related to the difference between bazel 
query
and 
aquery
or 
cquery
: rules have different inputs and configurations at each phase
a
or really just any specifics on how to make this conform more to the platonic ideal so that i can merge it
or just a clear statement that you don't see this being mergeable so i can close it
w
we don’t have those three graphs. instead, we have various graphs of rules based on the context
the digest of a target for 
test
will be different from the digest of a target for 
binary
a
i'm pretty sure that this solution just invalidates too coarsely as opposed to not invalidating correctly. are you able to describe why that's not the case?
w
if the goal is to determine whether “binary will redo something”, you cannot use the digest for 
test
a
?
but you do know the static build graph deps
w
they do not include things like pytest
a
and you can calculate a stable hash of the structs
w
or the version of your codegenerator
h
or just a clear statement that you don’t see this being mergeable so i can close it
Regardless of this discussion, the proposed first PR to refactor the 
--provides
and 
--documented
options is definitely mergeable and very valuable on its own.
a
the version of pytest changing does not affect the consumer of this information
w
@aloof-angle-91616: how do you know that though? that’s consumer specific
a
i made this PR specifically for Metals
w
i think that in this case folks are trying to use this for 
binary
, right?
ah, ok. so in that case it depends on a whole other set of things… which version of coursier, etc
but that’s my point. this is only the targets, and doesn’t include any of the extra information needed to do something with them in some context
if Metals is running export, it needs an export-specific digest
so this fingerprint is something, but it’s a subset, rather than superset
đź‘Ť 1
a
this is purely to calculate file invalidation
we don't have v2 export yet so i'm not sure why you would expect metals would be consuming something like export
right now metals tracks invalidation itself
you'll note olaf signed off on this
if you don't plan to accept it understanding that this format is a first step to an actual v2 export, please state that on the ticket so people can stop asking me about it
i don't have the time to devote to a whole design doc about this, i was trying to add a feature people were asking from pants for a while and that there's a clear use case for. i don't think it should be blocked. the only reason i'm pushing back is not ideological, i literally just do not have the time to have this discussion.
w
i’m just not sure how to explain to users what that fingerprint is safe to use for. the reason i raised query is that “fingerprint of raw inputs” is different from “fingerprint in some other context”, and query would allow for that subtlety. i’m not sure how to explain this one… 
file_fingerprint
with a big docstring…?
going to snooze for a bit to try and get some other things done.
Open in Slack
PreviousNext
Powered by