https://pantsbuild.org/ logo
Join Slack
Powered by
hey <@U06A03HV1> we get a graph cycle when consumi...
# development
h
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
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
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
hm.
h
I can push if you want
w
not sure it will help… your analysis seems right.
👍 1
let me look at it for a bit.
h
independently or pairing?
w
independently.
h
here's the diff
Sin título.diff
w
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
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
yea, sure. wrapped somehow. but TargetAdaptor to Target is almost just calling the constructor. so not sure it needs much wrapping.
h
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
yea, that’s probably it.
ends up used by RuleRunner as well.
👍 1
h
Ah, it needs to be 
QueryRule(ChosenLocalEnvironment, [Platform])
so we can inject 
Platform.create_for_localhost()
. Mind blown
w
hm… shouldn’t be…?
the 
Platform
is computed from the environment
so needing a platform to compute an environment is backwards i think
h
This is to figure out the environment inside 
plugin_resolver.py
-- we know it must be the local platform and 
__local__
env
w
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
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
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
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
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
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
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
ohhh, that's a simple workaround! great suggestion!
w
that’s the actual case you’re trying to test
h
it is indeed. thanks 🙂
w
so can use a 
python_sources
target or something, and then actually include that backend
Open in Slack
PreviousNext
Powered by