https://pantsbuild.org/ logo
b

bitter-ability-32190

05/26/2022, 4:48 PM
Proposal: Elide
file
and just use
resource
everywhere. Google Doc: https://docs.google.com/document/d/1pGib2ZJiyltfij0aZMm81S2XbATa8s6_ghuLnf2Xr_k/edit?usp=sharing
🚀 2
1
❤️ 2
Shoutout to @hundreds-father-404 for the help.
❤️ 1
I'm working on the code changes now to serve as a PoC
The PoC is yielding all sorts of fruit. Lots of improvements in behavior
h

hundreds-father-404

05/26/2022, 9:25 PM
@happy-kitchen-89482 @witty-crayon-22786 this has my unequivocal blessing, and Josh has a proof of concept working. But needs to be split up into prefactors to derisk review Before doing that, would be helpful to get your blessing when either of you get a chance
w

witty-crayon-22786

05/26/2022, 9:26 PM
yea, will take a look this afternoon
1
🙌 2
thank you!
h

happy-kitchen-89482

05/26/2022, 10:00 PM
Looking now
🙌 1
1
b

bitter-ability-32190

05/27/2022, 12:32 AM
I think @enough-analyst-54434 had thoughts from our thread as well
OK I think I'm going to revamp this one into a V2 to better capture the intent, and change the proposal a smidge. Prose isn't my strong suit, so thanks for indulging me 🙂
👍 2
OK V2: https://docs.google.com/document/d/1gdJjhfAiVymTTcV6sRmsHTvQrzgaFmXdY8-fkdFaqxc Now primarily focused less on the technical difficulties with 2 assets types, and focused on the core of the issue: there's only one type of asset: assets. Trying to force a choice between two types based on runtime behavior of code has failed us and our users. Also, both
file
and
resource
will be deprecated for a single unambiguous type
asset
.
💯 1
@witty-crayon-22786 @happy-kitchen-89482 @hundreds-father-404 @enough-analyst-54434 please take another look. Also @curved-television-6568 you might have an opinion too 😉
🙂 1
👀 1
h

hundreds-father-404

05/31/2022, 4:37 PM
Thanks! Proposal looks good to me, I like the edit to call it
asset
and clarify what's going in: this is about not forcing specific functionality into a target type
1
🙌 1
b

bitter-ability-32190

05/31/2022, 6:53 PM
I actually have a real-world use-case of relocating a
file
sitting outside our main code tree into our main code tree as a
resource
. It's impossible today, but would be trivially supported by
relocated_assets
in the propsal.
1
c

curved-television-6568

06/01/2022, 3:22 PM
Oh boy. I feel like a single
assets
target ought to be able to cover the use cases for
files
and
resources
, but I see all the nuances presented in the comments in that google doc, it makes my (granted, somewhat tired 🥱 ) head spin. 😛
w

witty-crayon-22786

06/01/2022, 6:21 PM
talked with Eric briefly on another topic, and one thing that occurred to us was that one potential option would be to say “we don’t support distinguishing between a resource/pkg_data/Classloader loaded file and a loose file: you’ll always get both”
h

hundreds-father-404

06/01/2022, 6:23 PM
Concrete example with a
go_package
depending on an `asset`: • at compilation time, we include the asset in the sandbox. Go will need it if the user is using
embed
. Otherwise, it will just be ignored by the compiler • at test time, we include the asset in the sandbox. The user might access it via Filesystem APIs. Otherwise, ignored
b

bitter-ability-32190

06/01/2022, 6:23 PM
That's kind of another way of slicing the proposal. An asset is simply a declaration of a thing, and you specify dependencies on that thing. How that thing gets packaged or copied or materialized is in the details of the action taking place, not in the details of how it is declared
w

witty-crayon-22786

06/01/2022, 6:23 PM
looses flexibility for test targets (i can’t avoid both packing a huge file into my app and placing it in the sandbox), but probably wouldn’t have any non-performance implications
1
h

hundreds-father-404

06/01/2022, 6:24 PM
Interesting point w/ performance of huge assets. Would immutable_digests make any difference?
w

witty-crayon-22786

06/01/2022, 6:25 PM
i don’t think so…? if you’re testing a pyoxidizer or go binary, then you would be embedding an X GB file into a binary
b

bitter-ability-32190

06/01/2022, 6:26 PM
i can’t avoid both packing a huge file into my app and placing it in the sandbox
Is that true? would the asset not be a dependency of the test, and therefore be place din the sandbox?
w

witty-crayon-22786

06/01/2022, 6:27 PM
@bitter-ability-32190: if you don’t have a distinction between types, and always place it in both places via https://pantsbuild.slack.com/archives/C0D7TNJHL/p1654107679405549?thread_ts=1653583739.420959&cid=C0D7TNJHL then we wouldn’t know to “only” put it in the sandbox
b

bitter-ability-32190

06/01/2022, 6:27 PM
WHy would it be in "both" though if it isn't a dependency of the foo_source? Just the test?
w

witty-crayon-22786

06/01/2022, 6:28 PM
either way probably… depends whether a test pulls in transitive or only direct dependencies. but can trigger the case either way.
but i guess you’re saying that a test would only be able to get a direct dependency on an
asset
as a loose file? i.e., a test target depending on an
asset
would always get it as loose?
h

hundreds-father-404

06/01/2022, 6:29 PM
foo_source
Go only has a single target type:
go_package
. So when you add a dep to it, it might be because the "source" code uses the embed (resources) API at compilation time. Or it could be because tests are reading it as a loose file w/ Filesystem API. By getting rid of
file
vs
resource
, Pants loses the ability to distinguish intent Other than the performance concern, I think That Is Just Fine though
b

bitter-ability-32190

06/01/2022, 6:30 PM
Or it could be because tests are reading it as a loose file w/ Filesystem API.
Help me understand why it'd be a dependency of the
go_package
if it is a test-time dependency
h

hundreds-father-404

06/01/2022, 6:31 PM
Because the
go_package
includes both the source files & test files. There is no
go_tests
, it is just
go_package
w

witty-crayon-22786

06/01/2022, 6:31 PM
can embed up to 2gb in a go binary: https://groups.google.com/g/golang-nuts/c/jFKGLbTv2XQ
👍 1
b

bitter-ability-32190

06/01/2022, 6:35 PM
Ahhh ok so there IS a case where one asset can be eithe rpackaged or sandbox filed. In that case I think both proposals make sense! 1. Only have one asset type: assets. Because to hit the point home, we shouldn't be defining how an asset is used at runtime by forcing the user to classify the asset at metadata time 2. In places where the usage is ambiguous (should hopefully be be rare. I can only think of tests which themselves must be compiled). We employ Stu's "edge metadata" idea. Allow users to choose which asset is loose vs packaged in the test binary
Which we can tackle 2 separately
w

witty-crayon-22786

06/01/2022, 6:36 PM
with the difference from bazel being that it would only be in cases where it was ambiguous?
b

bitter-ability-32190

06/01/2022, 6:37 PM
coke
w

witty-crayon-22786

06/01/2022, 6:37 PM
part of the reason why i think that bazel might have recursive
data
dependencies is for requests like this: https://github.com/pantsbuild/pants/issues/15500 … i need to better understand why Alonso wanted transitive
file
dependencies there.
The only alternative now is making the
file
dependencies being direct, which is not optimal as it forces downstream targets knowing about all of their upstream potential
file
dependencies.
will comment.
b

bitter-ability-32190

06/01/2022, 6:39 PM
Yeah transitive assets is an interesting case. Perhaps its the case that we dont inherit transitive assets in tests, as those should be packaged with their sources (in the case of Python we inherit them in the sandbox but not the target?)
For that ticket it seems like a cheating shortcut to include a dependency on the source so that all test environments inherit the asset. But thats my 2c
w

witty-crayon-22786

06/01/2022, 6:40 PM
For that ticket it seems like a cheating shortcut to include a dependency on the source so that all test environments inherit the asset.
i mean, that’s what the distinction between
resources
and
files
is
but i don’t think i understand what syntax you’re thinking of there.
b

bitter-ability-32190

06/01/2022, 6:43 PM
It's arguably very nuanced to say "all environments you test me in requires this specific loose file in the sandbox, therefore I'll add it to the source and expect the test environments to inherit the dep transitvely"
w

witty-crayon-22786

06/01/2022, 6:44 PM
it can be a thing if its library code (rather than the very top level of your app) which loads the loose file. although libraries expecting files in particular places is smelly
👃 1
b

bitter-ability-32190

06/01/2022, 6:46 PM
Exactly! Pants has some control over the environment we test your code in, but no control over the environment your code is deployed in. Having metadata provide weak hints is confusing and misleading. Then we end up throwing away your dependency when packaged, or not knowing how to transitively consume it because your weak hint doesn't jive with the current action
w

witty-crayon-22786

06/01/2022, 6:48 PM
sure. but the symmetry is supposed to be between a test and prod… it’s just not between
deploy_jar
,
go_binary
, or
pex_binary
and a test … rather, between an `archive`/`docker_image` and a test. but i think that that is orthogonal to the transitive-or-not discussion
b

bitter-ability-32190

06/01/2022, 6:50 PM
Ohh you're saying that if we have an archive which depends on a
pex_binary
, when we build the archive we throw away the files for the
pex_binary
but then transitvely inherit them back for the archive itself?! Talk about nuanced 😵
And now I think I understand how you see the world. I do think transitive dependencies is root of the evil here and is extremely relevant to the discussion. The distinction between the two asset types breaks down when you think about a singular target because most singular targets can only consume one asset "type". But when you blend in transitive dependencies it gets messy because a target which can only logically consume asset type A might be depended on by another which supports B, or A&B. Then what? We need a hint to know what the expectation is.
👍 1
w

witty-crayon-22786

06/01/2022, 6:54 PM
transitive dependencies are one aspect, yea. the other is some consuming targets actually caring about the distinction between the types
so those seem like the two big open questions. Alonso is in UTC i think, so should get more info on that one tomorrow.
oh, related to:
the other is some consuming targets actually caring about the distinction between the types
&&
In places where the usage is ambiguous (should hopefully be be rare. I can only think of tests which themselves must be compiled).
…we have https://www.pantsbuild.org/docs/reference-python_tests#coderuntime_package_dependenciescode already as an “edge metadata” case.
…which basically amounts to a “give me `files`” dependency.
b

bitter-ability-32190

06/01/2022, 7:06 PM
The world is clearer. I think one asset type is ideal, as it is a better representation of the intent of the user (here is an asset, how it is consumed will be called out at the consumption site(*) as trying to tell you that here is not always ideal or possible). Then the problem is truly, "how do we let the user tell us their intent". This also simplifies Pants code a lot because on the asset-generation side there's no duplication. On the asset consumption side theres no ambiguity on what the user desired because they told us. We no longer have to guess or make decisions for them
w

witty-crayon-22786

06/01/2022, 7:08 PM
to be clear:
On the asset consumption side theres no ambiguity on what the user desired because they told us.
…is already supposed to be the case. to the extent that we currently have unclear docs on this subject, we probably still will… but perhaps being contextualized on a target by target basis would help.
b

bitter-ability-32190

06/01/2022, 7:10 PM
I think it's ambiguous because being told a
python_source
depends on a
file
is meaningless (especially when packaging) unless you consider a
python_test
or an
archive
which is depended on by a
pex_binary
.
w

witty-crayon-22786

06/01/2022, 7:12 PM
maybe… depends whether the
data_dependency
equivalent is transitive
h

hundreds-father-404

06/01/2022, 7:12 PM
(this convo might be better as a video conference btw. so much nuance here)
b

bitter-ability-32190

06/01/2022, 7:15 PM
Well for the most part I got what I needed 😛 which was to understand better where that nuance is and why it isn't accounted for by using one asset type
How we solve that going forward might be worth a call though 😉
OK I think we can roll this forward with Eric's way of slicing it ("it's always in the sandbox"). It's a re-lensing of the proposal so I'll work with them on re-phrasing the docs as such. Hopefully the nuance of
runtime_dependencies
can be avoided and we can enjoy a simple target harmony. I have thoughts on our rollout/deprecation too, so stay tuned
👍 1
OK, I've responded to comments (highlighting that there's some misconceptions about how files get consumed in docker_image and archive), added a section about another way of thinking about this being that the asset simply is in all sandboxes where file/resource were. I also changed the deprecation/rollout plan to us adding asset as a new type, and not touching file/resource. It's gonna be painful for a release to support 3 asset types, but we can pull the plug soon after. This ensures we don't' knock over the current house of cards, and makes incremental PRs easy. I also moved it into our shared folder.
👍 2
Gahh, I keep forgetting about JVM's Pants-metadata-is-the-only-metadata case. We'll maybe wanna have a short meeting to find which idea we like best (or hate least)
👍 1
h

hundreds-father-404

06/07/2022, 2:32 PM
Stu was open to the idea of a call together
w

witty-crayon-22786

06/07/2022, 6:41 PM
yea, still think this might be worthwhile. can get a Doodle out to schedule it if you’re interested.
…sounds like folks are. i’ll do that.
1
🙌 1
b

bitter-ability-32190

06/07/2022, 6:45 PM
Much appreciated!
w

witty-crayon-22786

06/07/2022, 6:47 PM
@hundreds-father-404, @happy-kitchen-89482, @ancient-vegetable-10556, @bitter-ability-32190: https://doodle.com/meeting/participate/id/dNx9l8La (and anyone else who is interested)
1
!!! … this is potentially related to scoped dependencies. https://github.com/pantsbuild/pants/issues/12794
i had forgotten that that was the last time “producer decides” vs “consumer decides” (aka node vs edge) metadata had come up.
h

happy-kitchen-89482

06/07/2022, 7:35 PM
It might be good to have @enough-analyst-54434 participate
👍 1
w

witty-crayon-22786

06/07/2022, 7:36 PM
and @curved-television-6568: no pressure to attend. but you had had thoughts on the scoped dependencies ticket.
👍 1
booked this for tomorrow at noon PT: let me know if you’d like an invite
h

hundreds-father-404

06/08/2022, 5:51 PM
Any chance we could do it earlier, like 11 am PT? I would like to join, but will have to leave for the airport 15 minutes in. (I can always do voice call from Uber)
w

witty-crayon-22786

06/08/2022, 5:52 PM
i can move my other meeting if an hour earlier works for @happy-kitchen-89482 and @bitter-ability-32190
b

bitter-ability-32190

06/08/2022, 5:52 PM
👍
h

happy-kitchen-89482

06/09/2022, 6:51 AM
WFM
b

bitter-ability-32190

06/09/2022, 5:56 PM
Gonna be a smidge late. Also sorry ahead of time, gonna be slurping on lunch 😂