https://pantsbuild.org/ logo
b

bitter-ability-32190

03/08/2023, 7:35 PM
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

ancient-vegetable-10556

03/08/2023, 7:43 PM
👀
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

bitter-ability-32190

03/08/2023, 8:17 PM
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

ancient-vegetable-10556

03/08/2023, 8:18 PM
hm ok
b

bitter-ability-32190

03/08/2023, 8:18 PM
It's the last test too
a

ancient-vegetable-10556

03/08/2023, 8:18 PM
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

bitter-ability-32190

03/08/2023, 8:25 PM
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

ancient-vegetable-10556

03/08/2023, 8:25 PM
It’s superfluous
b

bitter-ability-32190

03/08/2023, 8:25 PM
Because it's empty, or just in general?
a

ancient-vegetable-10556

03/08/2023, 8:26 PM
I am not 100% across how rules are resolved when you pass in too many params
b

bitter-ability-32190

03/08/2023, 8:27 PM
"a sentinel, a platform, and a tar binary walk into a bar..."
🤣 1
a

ancient-vegetable-10556

03/08/2023, 8:27 PM
but first: do either of my suggestions fix the problem?
b

bitter-ability-32190

03/08/2023, 8:30 PM
I'll have to find out when I open the laptop next.
Right now it's mobile slack for me
a

ancient-vegetable-10556

03/08/2023, 8:30 PM
Fair enough 🙂
b

bitter-ability-32190

03/08/2023, 8:31 PM
Good old fashioned suspense
a

ancient-vegetable-10556

03/08/2023, 8:33 PM
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

bitter-ability-32190

03/08/2023, 10:32 PM
Unsurprisingly, changing the return type works
Is this a rule runner bug?
Ignoring params certainly seems like unintentional behavior to be 🙈
a

ancient-vegetable-10556

03/08/2023, 10:33 PM
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

bitter-ability-32190

03/08/2023, 10:34 PM
I'll light the bat signal @witty-crayon-22786
a

ancient-vegetable-10556

03/08/2023, 10:35 PM
I’m going to chalk this up as another case where
EnvironmentName
/`EnvironmentTarget` has difficult-to-grok consequences.
w

witty-crayon-22786

03/08/2023, 10:35 PM
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

ancient-vegetable-10556

03/08/2023, 10:36 PM
I have a half-formed suggestion in my head that probably deserves consideration re. rules that expect an explicitly-provided
EnvironmentName
(or
EnvironmentTarget
)
w

witty-crayon-22786

03/08/2023, 10:38 PM
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

bitter-ability-32190

03/09/2023, 3:26 AM
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

witty-crayon-22786

03/09/2023, 3:49 AM
Did you install a queryrule for the query you are trying to do?
b

bitter-ability-32190

03/09/2023, 3:49 AM
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

witty-crayon-22786

03/09/2023, 5:49 PM
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

ancient-vegetable-10556

03/09/2023, 5:50 PM
(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

bitter-ability-32190

03/09/2023, 5:54 PM
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

witty-crayon-22786

03/09/2023, 6:02 PM
i can file a ticket about that.
2 Views