https://pantsbuild.org/ logo
Powered by
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
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.
2 Views
Powered by