https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-father-404

09/06/2022, 8:47 PM
hey @witty-crayon-22786 we get a graph cycle when consuming your PR because
Get(GeneratedTargets, GenerateTargetsRequest)
inside
resolve_target_parametrizations
. Even that code path isn't triggered in bootstrapping for discovering env targets, the rule graph doesn't know that
We need a
WrappedTargetRequest -> WrappedTarget
that doesn't allow for target gen. Any suggestions? Lmk if you want to pair
I'm not really sure how parametrization plays into this. I don't think we need to support
parametrize
with env target definitions, right?
w

witty-crayon-22786

09/06/2022, 8:48 PM
no, we don’t need to support
parametrize
with env targets.
hey @witty-crayon-22786 we get a graph cycle when consuming your PR because
Get(GeneratedTargets, GenerateTargetsRequest)
inside
resolve_target_parametrizations
.
in which context? what edit(s) have you made?
h

hundreds-father-404

09/06/2022, 8:51 PM
1. Now,
EnvironmentName
you added is the same from my old
ResolvedEnvironmentAlias
2. For now, I inject
EnvironmentName(None)
whereas you did
EnvironmentName()
3.
python_bootstrap.py
needs to request
EnvironmentTarget
as a rule param, whereas before it was doing
Get(EnvironmentTarget, EnvironmentRequest(LOCAL_MATCHER)
. *Otherwise a really bad rule graph error. I suspect this is because it's used in bootstrapping? That causes a rule graph issue. I can fix it by commenting out
Get(GeneratedTargets, GenerateTargetsRequest)
inside
resolve_target_parametrizations
w

witty-crayon-22786

09/06/2022, 8:53 PM
hm.
h

hundreds-father-404

09/06/2022, 8:53 PM
I can push if you want
w

witty-crayon-22786

09/06/2022, 8:55 PM
not sure it will help… your analysis seems right.
👍 1
let me look at it for a bit.
h

hundreds-father-404

09/06/2022, 8:56 PM
independently or pairing?
w

witty-crayon-22786

09/06/2022, 8:56 PM
independently.
h

hundreds-father-404

09/06/2022, 8:57 PM
here's the diff
w

witty-crayon-22786

09/06/2022, 8:58 PM
so… given that we don’t need target generation or parametrization for this case, you might want to have the environment target stuff directly consume a
TargetAdaptor
…?
h

hundreds-father-404

09/06/2022, 9:00 PM
that's roughly what I'm proposing. we do need it to be a
Target
so we get Target API benefits like field validation. Otherwise consuming sites will be totally untyped I'm thinking
WrappedTargetRequest -> WrappedTargetForBootstrappingOnly
or something
w

witty-crayon-22786

09/06/2022, 9:02 PM
yea, sure. wrapped somehow. but TargetAdaptor to Target is almost just calling the constructor. so not sure it needs much wrapping.
h

hundreds-father-404

09/06/2022, 9:03 PM
yeah, and determining which target type class to use etc. I'll DRY that via a helper function. Cool, I'm unblocked then
Alright @witty-crayon-22786 I got it working to inject
EnvironmentName(None)
everywhere 🚀 https://github.com/pantsbuild/pants/compare/main...Eric-Arellano:merge-env-name?expand=1 the next step is to update those sites to instead do
session.product_request(ChosenLocalEnvironmentName, [Params()])
. Do you know offhand where I define that
QueryRule
?
engine_initializer.py
?
w

witty-crayon-22786

09/06/2022, 9:50 PM
yea, that’s probably it.
ends up used by RuleRunner as well.
👍 1
h

hundreds-father-404

09/06/2022, 10:00 PM
Ah, it needs to be
QueryRule(ChosenLocalEnvironment, [Platform])
so we can inject
Platform.create_for_localhost()
. Mind blown
w

witty-crayon-22786

09/06/2022, 10:01 PM
hm… shouldn’t be…?
the
Platform
is computed from the environment
so needing a platform to compute an environment is backwards i think
h

hundreds-father-404

09/06/2022, 10:02 PM
This is to figure out the environment inside
plugin_resolver.py
-- we know it must be the local platform and
__local__
env
w

witty-crayon-22786

09/06/2022, 10:02 PM
the environment codepath is the only one that should be able to use
Platform.create_for_localhost()
directly, when it determines that it should based on… settings?
h

hundreds-father-404

09/06/2022, 10:03 PM
I'm pretty confident this is right. I'm gonna keep trudging forward and hope to have something up in a few minutes. Rule graph is happy now 🙂
w

witty-crayon-22786

09/06/2022, 10:03 PM
This is to figure out the environment inside
plugin_resolver.py
-- we know it must be the local platform and
__local__
env
seems like it should be something like:
Copy code
QueryRule(EnvironmentName, [EnvironmentMatcher])
…?
oook!
h

hundreds-father-404

09/06/2022, 10:08 PM
inside plugin resolver 😎
Copy code
❯ ./pants --no-pantsd
ChosenLocalEnvironmentName(val='default_env')
And your multiple params code works indeed! 🎉 Hardcoding
Get(TestResult, {field_set: TestFieldSet, EnvironmentName("docker"): EnvironmentName})
inside
test.py
results in
Copy code
❯ ./pants test src/python/pants/util/strutil_test.py
18:05:55.36 [INFO] //:default_env
18:05:56.50 [INFO] Initializing scheduler...
18:05:57.04 [INFO] Scheduler initialized.
18:05:57.37 [INFO] //:docker_env
Super cool. Now just to figure out how to extract the env from each FieldSet 🙂
🙌 2
@witty-crayon-22786 any ideas for how to work around this in a test? The integration test
test_unrecognized_build_file_symbols_during_bootstrap
now fails because the
StreamingWorkunitHandler
gets created in
local_pants_runner.py
by calling
determine_bootstrap_environment(self.graph_session.scheduler_session)
, and that fails on the unrecognized symbol. Whereas we wanted
./pants --version
to succeed to show we can safely handle unrecognized target types I believe this failure is legit -- we want StreamingWorkunitHandler to fail because we are past the bootstrap stage (plugin resolver) We no matter what create
StreamingWorkunitHandler
, so I don't think there's any way around it. It fails regardless of the goal like
help
or
--version
I had the idea of parsing the stacktrace to make sure the failrue doesn't come from
plugin_resolver.py
, but that doesn't work becuase it's a generic
select
in the rule graph trace
w

witty-crayon-22786

09/07/2022, 4:44 PM
the symbol in this case is actually bad
it shouldn’t be 😃
because if it’s actually bad, it’s ok for things to fail post-bootstrap
h

hundreds-father-404

09/07/2022, 4:45 PM
agreed. my goal is to make sure our plugin resolver stage can handle unrecognized symbols, and I don't know how to test that anymore
our prod code is all valid, I only don't want to lose the regression test
w

witty-crayon-22786

09/07/2022, 4:45 PM
the symbol in this case is actually bad
a realistic example would be that the symbol exists in a backend which isn’t loaded initially
h

hundreds-father-404

09/07/2022, 4:45 PM
ohhh, that's a simple workaround! great suggestion!
w

witty-crayon-22786

09/07/2022, 4:46 PM
that’s the actual case you’re trying to test
h

hundreds-father-404

09/07/2022, 4:46 PM
it is indeed. thanks 🙂
w

witty-crayon-22786

09/07/2022, 4:46 PM
so can use a
python_sources
target or something, and then actually include that backend