https://pantsbuild.org/ logo
Join Slack
Powered by
<@U051221NF> added `run_in_sandbox` field to `pex_...
# development
b
@happy-kitchen-89482 added 
run_in_sandbox
field to 
pex_binary
, which currently hasn't been put in a stable release yet. https://github.com/pantsbuild/pants/pull/15849 reverts that change in lieu of another change that offers the same behavior, but in a different way. We need to figure out our path forward for the current release: 1. Leave the feature, then do a full deprecation cycle and remove 2. Change the field to 
experimental_...
so we could pull the plug without deprecation 3. Get PR 15849 in ◦ I'm only waiting on docs addition and an open TODO I want feedback on As the author of the PR, my vote will obviously be 3 😊 , but I'd like us to be intentional on our path forward.
CC @sparse-lifeguard-95737 who cares very much for their org 🙂
s
(Copying from the other thread) to be clear, I don’t really care either way about 
run_in_sandbox
on 
pex_binary
vs. un-sandboxed 
run
directly on a python script - I can make either one work for my purposes. I’ve had people breathing down my neck about django management CLI + pants not working as expected, and was going to point at the new 
run_in_sandbox
field and say “look it’s going to work soon!” when I saw chatter here about possibly reverting the addition of that field in 2.13. I don’t mind if it’s reverted, I just want to know if it’s going to be reverted so I don’t bait-and-switch my team
👍 2
h
bump on this @happy-kitchen-89482 -- I recommend we don't land the 
run
change in 2.13 because we already are at a0 and have so many big changes but @bitter-ability-32190 and @sparse-lifeguard-95737, how do we feel about keeping 
run_in_sandbox
but calling it 
experimental_run_in_sandbox
so that we can remove it more easily?
s
experimental works for me 👍
b
Thats my option 2 🙂 No qualms here. Also FWIW if Dan can stomach the pain, the escape-hatch is an in-repo plugin which has this functionality (its what we're using at my work-repo)
I think as long as we're ready to brace the inevitable question(s) of "where did this thing go? I was using it!", no harm in renaming
h
With 
experimental_run_in_sandbox
, I still recommend we don't fully delete the field in the next Pants release. At worst, make it a no-op and then deprecate the field @bitter-ability-32190 I vaguely remember you have an option so that running a 
pex_binary
runs it the old way, where we run from loose sources, right? If so, then I think we could still have 
experimental_run_in_sandbox
do useful things when the old semantics are used? The intent with the name 
experimental_
is more than anything to discourage people from relying on the feature
b
With the new change, the user could/should be running the python_source, in which case the semantics of in-repo with an option to run in sandbox as well
h
I'm not sure that we need to deprecate that field. There may be reasons for people to want to build a pex and run it "inline"
b
@happy-kitchen-89482 I'm not sure I understand what that means 😵 The code being run is packaged into the PEX. it is neither in a sandbox or in repo
At worst, make it a no-op and then deprecate the field
Ah I missed that. I don't like the gymnastics this switchover is going to take in the code, but in Pants 2.16 or whatever hopefully it'll be behind us?
I also dont want to inundate our users with too many new knobs/levers
h
Oh, right, makes sense. We would be changing the implementation of "run" in that case.
b
Yes, however with how its implemented the change in behavior is opt-in. No one will upgrade and stumble into the new behavior, they have to issue their command with a different spec than before
h
Is that always the case? people could have been using the path/to:target spec before
b
h
Well but previously we weren't building the sources into the pex, and now we would be
IIUC
b
Ah yes! And here's my trick: https://github.com/pantsbuild/pants/blob/d7589fc1cc4d746b51f9faff06ea80904b41f8a2/src/python/pants/engine/internals/specs_rules.py#L487 So ultimately, • If you use the pex binary target address behavior wont change. You get warned and theres an option to switch to new behavior if you want • If you use the file as your spec, same as the above. We dont issue an ambiguity error and we purposefully choose the binary to run • If you're using a file address and theres no binary, this is a new thing so whatever goes
❤️ 1
So, any consensus?
I just don't want us to decide by not deciding 😅
h
I think leaving the field as-is (or renaming it) and doing a deprecation cycle is fine
b
Ah, well I'm bummed that this won't make it in 2.13 for our users (and for my own repo). As an aside, after an upgrade to 2.14 if a user runs 
./pants run path/to/file.py
and has 
run_in_sandbox
set, they'll 3 get warnings 🙈 .
h
We can backport, no?
To 2.13?
b
What do you mean?
h
Re
Ah, well I'm bummed that this won't make it in 2.13 for our users (and for my own repo).
b
Are you suggesting we cherry-pick the feature from 
main
to 2.13? And then in 2.13 add, but also deprecate 
run_in_sandbox
field?
h
Oh I see what you mean. If we get it all in in 2.13 we don't need to deprecate and can just remove it. I'm fine with that.
🙂 1
b
So I know @hundreds-father-404 had reservations
h
my fear was that 2.13 already has so much in it, and this is a big change
b
I can help us toss something else out 😂
h
I'm not well positioned to chime on on that, so I will have to remain agnostic on the question of whether we get this into 2.13 or 2.14, but I'm pro the change either way.
h
I guess 2.13 doesn't have a ton of user API changes. The big ones are: • new CLI specs • 
filter
goal deprecated, which impacts scripting etc So this seems fine from a User API disruption perspective. Lots of internal changes tho, so it may delay time to stabilizing the release. And @bitter-ability-32190 I still think we need to make the followup changes like what to do with 
--tailor-pex-binary-targets
in the same release. It's coupled
b
I can work on tailor ASAP
❤️ 1
This also is a big win for the debug stuff im adding because sandboxing the sources makes debugging silently impotent
👍 1
h
did we decide what to do with 
tailor
?
b
Too many threads 🙈
My stance was/is "Python is way more likely to have people's scripts than packaged deliverables, and theres no easy way to tell the latter. So lets nix it, and let 
pex_binary
be intentional and opt-in"
h
Do you mean "tailor generates pex_binary targets" should be opt-in? Or we remove that functionality entirely from tailor? I'm pro the former, against the latter, because people have relied on tailor for this
b
As long as it isnt on-by-default I'm game
h
I think that makes sense
We already have an option for this, we just need to change the default
👍 1
b
Alrighty, here's the current Plan of Attack: • My change(s) goes into 2.13. You can 
run
a 
python_source
• A handful of deprecation/removal/warnings regarding current and future behavior • New field on 
python_source
and friends 
run_goal_use_sandbox
, defaulting to True (to maintain backwards compat) ◦ @sparse-lifeguard-95737 is this sufficient? We mused about a global default a la 
pex-binary-defaults
, but I'd like to avoid that with 2.14's `__default__`s feature.
h
The key insight @bitter-ability-32190 and I had yesterday is we're being extra careful to remove risk of backwards compatibility breaking. For example, we believe 
run_in_sandbox=False
is probably the best default -- but we are no longer immediately jumping to that, as it would be breaking As a result, I'm less concerned than I was before
👍 1
b
Open in Slack
PreviousNext
Powered by