Weird issue with `rule_runner`... :thread:
# development
b
Weird issue with
rule_runner
... 🧵
I have a rule that has an "if local_env do X, otherwise do Y". In order to test the Y behavior easily I split it into two rules:
Copy code
@rule
async def my_rule(
    env_tgt: EnvironmentTarget
) -> ReturnType:
    if env_tgt.val is None or isinstance(env_tgt.val, LocalEnvironmentTarget):
        return ReturnType(<local>)

    result = await Get(
        ReturnType,
        _DoThingInNonlocalEnv()
    )

    return result

@rule
async def do_the_thing_in_docker(
    _: _DoThingInNonlocalEnv, platform: Platform, tar_binary: TarBinary
) -> ReturnType:
    return ReturnType(<docker>)
With:
Copy code
class _DoThingInNonlocalEnv:
    pass
In my test I have:
Copy code
rule_runner.write_files(
        {
            "BUILD": "local_environment(name='local')",
        }
    )
    rule_runner.set_options(["--environments-preview-names={'local': '//:local'}"])
    result = rule_runner.request(ReturnType, [_DoThingInNonlocalEnv()])
    assert result <is docker result>
But the rule runner is returning a value from the first rule. Specifically the local return type value
a
šŸ‘€
Looking at the actual test, I think your test is asserting that an invalid result is returned?
(alternatively, I can’t actually figure out which of the tests in the PR is the one that you’ve turned into pseudocode)
b
That's true, however it fails because the result is my local python
Which because of the request type, shouldn't be the case
a
hm ok
b
It's the last test too
a
yup, I figured from that latter description
I have a theory here!
So I think
EnvironmentTarget
can be resolved automatically if there’s an
EnvironmentName
param in the relevant subgraph, and in this case, there is. That’d mean there’s two rules that fulfil the params you pass in.
Can you try making a simple wrapper class around
PythonBuildStandAloneBinary
, and making
download_python_binary
return that instead?
Alternatively, make a wrapper around
EnvironmentTarget
and update
get_python_for_scripts
to take that wrapper type instead of
EnvironmentTarget
b
How would the first rule fulfill the param types since I'm including the sentinel type as a param? That's the part I don't understand
a
It’s superfluous
b
Because it's empty, or just in general?
a
I am not 100% across how rules are resolved when you pass in too many params
b
"a sentinel, a platform, and a tar binary walk into a bar..."
🤣 1
a
but first: do either of my suggestions fix the problem?
b
I'll have to find out when I open the laptop next.
Right now it's mobile slack for me
a
Fair enough šŸ™‚
b
Good old fashioned suspense
a
My working theory is that both rules can be run under the universe of params that are available to them, and so it’s running the first one that was registered. Whether that’s usefully correct or not is an open question.
b
Unsurprisingly, changing the return type works
Is this a rule runner bug?
Ignoring params certainly seems like unintentional behavior to be šŸ™ˆ
a
I’m not going to say it’s expected behaviour, but it’s definitely plausible behaviour once you know how rules are resolved
Going to ping in @witty-crayon-22786 now! šŸ™‚
b
I'll light the bat signal @witty-crayon-22786
a
I’m going to chalk this up as another case where
EnvironmentName
/`EnvironmentTarget` has difficult-to-grok consequences.
w
It’s superfluous
yes, this. values which are not used by declared queries will be ignored.
did you declare a Query of the shape you’re trying to execute?
a
I have a half-formed suggestion in my head that probably deserves consideration re. rules that expect an explicitly-provided
EnvironmentName
(or
EnvironmentTarget
)
w
a magical bit here is that unless you pass
RuleRunner(.., inherent_environment=None)
, all queries used in a
RuleRunner
implicitly get a default
EnvironmentName
injected
šŸ‘† 1
ā˜ļø 1
> It’s superfluous
yes, this. values which are not used by declared queries will be ignored.
more on this: this is at least in part because goal rules all have a static
Query
which provides them with everything that they _might_ need, and
run_goal_rule
always passes all params that they might need. the matching from ā€œwhich parameters you passedā€ to ā€œquery to useā€ does a subset match. that could probably be changed with some finagling.
b
Welp I'm still lost šŸ™ƒ
But I suppose I have a hacky workaround. I do hate dirtying the prod code for tests sale though
w
Did you install a queryrule for the query you are trying to do?
b
Yeah. The PR is in the thread
To re-map it back onto the original thread code:
Copy code
QueryRule(ReturnType, [EnvironmentName, _DoThingInNonlocalEnv])
(also tried without the EnvironmentName)
w
yea, i think i see what happened here. the
RuleRunner(.., inherent_environment=..)
means that you end up with a query like
QueryRule(ReturnType, [EnvironmentName, _DoThingInNonlocalEnv])
(because it rewrites your queries)… it’s then using the rule which requires the fewest number of parameters to solve. so disabling the inherent_environment would fix this, but you’d have to actually explicitly pass the EnvironmentName for other things you were trying to execute
a
(Generally speaking, I think it’s best to avoid writing rules that pass around non-wrapped `EnvironmentName`/`EnvronmentTarget` objects if you can avoid it — there’s less chance of running into magic)
b
I thought I tried that and hit the same issue... Let me try again
Yeah so I tried with
inherent_environment=None
, then in the query rule passing the name type, and in the runner request also passing the name. Same issue
w
k… sorry for the trouble. at a fundamental level, this is caused by: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1678315342820359?thread_ts=1678304144.161169&amp;cid=C0D7TNJHL
i can file a ticket about that.