pantsbuild #development

Channels

welcome

github-notifications

development

hundreds-breakfast-49010

02/19/2020, 8:34 PM

is that deliberate? or did these two types get out of sync somehow? maybe @early-needle-54791 has some insight into this?

aloof-angle-91616

02/19/2020, 8:34 PM

src/python/pants/engine/platform.py

should have just linux and darwin?

hundreds-breakfast-49010

02/19/2020, 8:35 PM

process_execution/src/lib.rs

aloof-angle-91616

02/19/2020, 8:35 PM

platform constraints should have a none case

early-needle-54791

02/19/2020, 8:35 PM

Yeah its deliberate, the None type signals that a request is cross platform compatible

early-needle-54791

02/19/2020, 8:36 PM

But that isn’t needed on the python side.

hundreds-breakfast-49010

02/19/2020, 8:37 PM

the changes I want to make might break that assumption. I'm trying to encode what platform an EPR did in fact run on, which can't be None

aloof-angle-91616

02/19/2020, 8:37 PM

@early-needle-54791 is that part of the remote execution api or do we just use it elsewhere in our rust code

aloof-angle-91616

02/19/2020, 8:37 PM

I'm trying to encode what platform an EPR did in fact run on, which can't be None

that sounds like it's a different data point and might deserve a new enum?

hundreds-breakfast-49010

02/19/2020, 8:37 PM

maybe

aloof-angle-91616

02/19/2020, 8:38 PM

this is why we have the distinction between

Platform

and

PlatformConstraint

in our python code

aloof-angle-91616

02/19/2020, 8:38 PM

it may make sense to just use that terminology

aloof-angle-91616

02/19/2020, 8:38 PM

i also like @early-needle-54791's mention of "materialized platform"

hundreds-breakfast-49010

02/19/2020, 8:40 PM

so it sounds like the type that is currently called

Platform

in rust should really be called

PlatformConstraint

hundreds-breakfast-49010

02/19/2020, 8:40 PM

and it corresponds to python

PlatformConstraint

not python

Platform

aloof-angle-91616

02/19/2020, 8:40 PM

i agree

hundreds-breakfast-49010

02/19/2020, 8:41 PM

okay, I'm gonna submit a small pre-work commit that renames this type in rust then, since I think it makes it easier to reason about how the rust and python sides correspond

👌 1

hundreds-breakfast-49010

02/19/2020, 11:01 PM

https://github.com/pantsbuild/pants/blob/master/src/rust/engine/process_execution/src/lib.rs#L343 <- is there a reason that this trait fn has an explicit, panicking default implementation on the trait, rather than just no default implementation? @early-needle-54791

hundreds-breakfast-49010

02/20/2020, 12:05 AM

does anyone who understand bazel remote execution well know what the "target_platform" string that gets set here: https://github.com/pantsbuild/pants/blob/master/src/rust/engine/process_execution/src/remote.rs#L913 is used for?

hundreds-breakfast-49010

02/20/2020, 12:06 AM

maybe @average-vr-56795?

hundreds-breakfast-49010

02/20/2020, 12:40 AM

hm, do we actually need to support the use case where a

MultiPlatformExecuteProcessRequest

has multiple different

ExecuteProcessRequests

👍 1

hundreds-breakfast-49010

02/20/2020, 12:40 AM

I don't think we're currently doing this

hundreds-breakfast-49010

02/20/2020, 12:41 AM

the only time we actually use

MultiPlatformExecuteProcessRequest

in python code directly is when we instantiate it in

pex.py

, and there's we're only creating a single

ExecuteProcessRequest

hundreds-father-404

02/20/2020, 12:41 AM

It would be a much simpler API to be 1-1 with EPR imo

hundreds-breakfast-49010

02/20/2020, 12:41 AM

on the other hand, I can see how in principle you might want some thing about an EPR to be different depending on platform- maybe

hundreds-breakfast-49010

02/20/2020, 12:42 AM

@hundreds-father-404 yeah I've been digging into the rust code around

process_execution

and there's a lot of complexity that can be gotten rid of if we don't have multiple copies of the same EPR per-platform

👍 1

hundreds-breakfast-49010

02/20/2020, 12:46 AM

@witty-crayon-22786 do you have any thoughts on this?

witty-crayon-22786

02/20/2020, 12:54 AM

@hundreds-breakfast-49010: i would suggest reading through @early-needle-54791's design doc on this topic, and (if he has time) scheduling some time to discuss it

👍 1

witty-crayon-22786

02/20/2020, 12:56 AM

i don't have sufficient context to explain the design, but there was a reason that it chose to differentiate platforms: https://docs.google.com/document/d/1mdMXWQ4qLhTmnRscq-kx8LI6HY0ivYkTVqpJCVkDdjQ/edit

witty-crayon-22786

02/20/2020, 12:56 AM

we do know that there are followup tweaks to that design necessary: https://github.com/pantsbuild/pants/issues/8090#issuecomment-536051780