https://pantsbuild.org/ logo
Join Slack
Powered by
<@U0N6C2Q9F>, <@UB2J9BQA0>: i think that my next s...
# development
w
@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
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
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
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
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
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
@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
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
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
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
(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
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
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
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.
Open in Slack
PreviousNext
Powered by