https://pantsbuild.org/ logo
w

witty-crayon-22786

08/29/2022, 5:23 PM
@fast-nail-55400, @hundreds-father-404: i think that my next step for https://github.com/pantsbuild/pants/issues/13682 will be attempting to move
Platform
from a singleton to a param, and then possibly looking into consuming it to construct
Process
… will that step on any toes?
…oh, ouch. i missed that this is not injected at all currently. that’s going to be more churn than i realized.
mostly used like:
Copy code
src/python/pants/jvm/resolve/coursier_setup.py
263:        coursier_subsystem.get_request(Platform.current),
1
so i think that the answer to my question is that i should probably work with Eric on the design of the environment id before a sweeping change like this.
actually (continuing to think out loud): the
Environment
is important, but almost no callsites should actually consume “the entire environment”, or even the environment id: they will almost all consume things derived from the environment. and the
Platform
is one of those things (i.e., the environment id gets pushed down to select the environment, and the
Platform
is computed from the environment config).
so i think that it makes sense to move the
Platform
from singleton to parameter.
h

hundreds-father-404

08/29/2022, 5:39 PM
would it help for me to push my WIP for adding
local_environment
? Rn, it's global, no per-target settings. The plan is still that rules will request something like how
pex_cli.py
already requests the type
PexEnvironment
. That rule will now request which environment to use to populate those env vars
w

witty-crayon-22786

08/29/2022, 5:39 PM
but i’ll hold off on integrating with
Process
quite yet, because we need to figure out how the platform and docker image interface with that for @fast-nail-55400’s change
That rule will now request which environment to use to populate those env vars
sure… temporarily, that makes sense. the Environment (id) should become a global param here: https://github.com/pantsbuild/pants/blob/489b1e2fb1ffee2864cf7dc1308bd2afa4fa2f47/src/python/pants/init/engine_initializer.py#L87-L88
the
Get
syntax will be used for changing the environment in certain subgraphs, but it will still be a global/default param
@hundreds-father-404: i don’t think that these changes will collide, so i’ll start on this
Platform
change
f

fast-nail-55400

08/29/2022, 5:53 PM
the docker change can wait for
Platform
churn before actually using it.
as it currently stands, it would just be passed through as the
docker
--platform
option.
w

witty-crayon-22786

08/29/2022, 5:54 PM
the connection here is to platform, yes, but the source of the actual image to use will be the Environment
…that bit should definitely wait on Eric’s stuff
f

fast-nail-55400

08/29/2022, 5:55 PM
I can just rely on
Process.platform
for the short term and no big deal to migrate to whatever it changes to
so feel free to churn away
h

hundreds-father-404

08/29/2022, 7:48 PM
@witty-crayon-22786 fyi what I have so far for `local_environment`: https://github.com/pantsbuild/pants/pull/16683 This is actually fairly representative of the first past. I didn't finish wiring it up to all the rules, but it shows the technique. Once I finish this PR, then I was thinking: 1. Migrate all relevant options to work with
local_environment
, whereas right now it's only
[python-bootstrap]
2. Allow individual targets to change the environment. (requires more design work) 3. introduce
docker_environment
target We may want to have me do #2 and #3 first, as they are more conceptual and blocking imo. Maybe Chris could help with #1
w

witty-crayon-22786

08/29/2022, 8:09 PM
yea, 2 and 3 first would make sense. and yea to asking Chris to help with 1 once that’s unblocked.
actually (continuing to think out loud): the
Environment
is important, but almost no callsites should actually consume “the entire environment”, or even the environment id: they will almost all consume things derived from the environment. and the
Platform
is one of those things (i.e., the environment id gets pushed down to select the environment, and the
Platform
is computed from the environment config).
i’ve begun on this
Platform
change, but it looks like it is going to need to introduce an initial cut at some sort of EnvironmentId or EnvironmentMatcher. needing to do it early is for annoying rule-graph reasons, but i do think that it will be the long term factoring anyway, so maybe that’s a blessing in disguise
so @hundreds-father-404: we might want to talk more about what “matching an environment” means. as a straw design: the param that flows down the graph could be an “EnvironmentMatcher”, which deterministically matches a single environment. it’s not really an “id” per-se, because maybe it’s just going to match the “<local>” environment (whichever that happens to be), although it might also be a specific docker image. but the resolution from Matcher to an actual environment will happen in your rules
it won’t really be part of any public APIs until your (2), so we don’t really have to get it perfect.
h

hundreds-father-404

08/29/2022, 8:27 PM
the issue is going to be migrating to per-target, #2. Right now, to determine the environment is easy because it's global. Soon, I think the consumers like
pytest_runner.py
are going to need to ask for the environment for a particular target, i.e. give input
w

witty-crayon-22786

08/29/2022, 8:28 PM
Right now, to determine the environment is easy because it’s global. Soon, I think the consumers like
pytest_runner.py
are going to need to ask for the environment for a particular target, i.e. give input
that’s what my current change begins to do
see my “i’ve begun on this
Platform
change” comment
but yea, that maybe helps us think about what an
EnvironmentMatcher
contains. basically, a root target like
python_tests
is going to have a Field which can be converted into an
EnvironmentMatcher
and there will be an inherent/global
EnvironmentMatcher
that matches the local environment (which i need to introduce currently)
let me know if you want to meet briefly about this
(relatedly: I'm regretting having gone with the shorter name and reserving
pants.engine.environment.Environment
for env vars)
h

hundreds-father-404

08/29/2022, 8:38 PM
(relatedly: I'm regretting having gone with the shorter name and reserving pants.engine.environment.Environment for env vars
It's annoying to change the Plugin API, but I think we can consider it. Much less stable than user API
w

witty-crayon-22786

08/29/2022, 8:39 PM
yea. as confusing as it is, i might put
EnvironmentMatcher
in that module, and then we can deprecate/move
Environment
, etc later if things work out.
@hundreds-father-404: does the above make sense? if so, i don’t think i need a call right now unless you do
h

hundreds-father-404

08/29/2022, 8:44 PM
I think so. I think though that we need to have that EnvironmentMatcher go pretty deep in the stack trace? Look at
python_bootstrap.py
and the rule to return
PythonBootstrap
. That impacts a lot, including any PEX that gets built. If one target uses env1, and other uses env2, then we need to build
pytest.pex
twice I think. I'm not totally clear on how we will pass the target down into
python_bootstrap.py
w

witty-crayon-22786

08/29/2022, 8:45 PM
the rule graph allows for that automatically.
https://github.com/pantsbuild/pants/pull/16682 needs an update for the EnvironmentMatcher concept, but if you look at the changes to
src/python/pants/init/engine_initializer.py
in there, imagine that rather than a
Platform
at the root, there is an
EnvironmentMatcher
at the root
👍 1
that param becomes available throughout the graph. and if it needs to be different in a particular subgraph, you’d use:
Copy code
await Get(TestResult, {field_set: TestFieldSet, env_matcher: EnvironmentMatcher})
…to change it in the subgraph.
👍 1
intermediate `@rule`s don’t need to care: the environment becomes part of their identity, but they don’t need to know about it.