[RLLib] (P1 regression) Fixing view requirements in compute actions

[Bam4d]

Why are these changes needed?

@sven1977 This is a regression of a previous bug I raised. Griddly needs this to work!

The view requirments for inference do not just include the model's view requirements.

get_inference_input_dict needs to return the input dict with all of the view requirements for the whole policy rather than just the model, otherwise all the used_for_compute_actions get ignored.

Related issue number

This bug is a regression of this fix

Regression from this fix: #14386
Closes #14385

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[duburcqa]

@sven1977 Do you think this can be merged before releasing 1.4.0 ?

[Bam4d]

@sven1977 Do you think this can be merged before releasing 1.4.0 ?

The previous fix (which then broke) wasn't merged in 1.3.0, I'd really like to be able to tell griddly users to download a stable release rather than the unstable wheels to get anything to work.

[Bam4d]

@sven1977 @duburcqa Super bummed this didn't make it into 1.4.0. Whats the hold-up with it? Is there anything I can do to get this through?

[duburcqa]

I agree with you, it is quite a pity it did made it in 1.4.0. it is a very annoying bug. It took me way too long to understand why my code was not working, and there is no clean way to get around it apart from patch ray.rllib.

[sven1977]

Hey @Bam4d , @duburcqa , I'm really sorry for the hold-up. However, please be aware that we have limited resources to get these PRs reviewed and merged. Also, if you have noticed, your PR has some failing test cases, which someone has to look into (and debug) before we can merge this! If all tests were pass, we could have merged this some time ago.

[sven1977]

Ok, I looked into this again. Thanks for the fix - again :)
The problem is that some policies in RLlib (e.g. QMIX) are based right off the base Policy class, so they "miss" the code, where we unify the policy's and the model's view requirements. I think this could be fixed by doing this here:

        # vv Do merge here if not already done.  vv
        view_reqs = policy.model.view_requirements if \
            getattr(policy, "model", None) else policy.view_requirements

        input_dict = {}
        for view_col, view_req in view_reqs.items():

[sven1977]

All failing tests passing locally now, just waiting for CI ...

[sven1977]

Thanks for raising this issue and for the suggested fix @Bam4d .
Had to make 2 small adjustments for this to also work with legacy algos (e.g. QMIX). Looks all good now.

[duburcqa]

@sven1977 Amazing ! Thank you for fixing and merging this PR 👍