Match AI2-THOR's step kwargs support

[mattdeitke]

Currently when running the following in AllenAct

self.env.step(...)

you have to pass in a dict:

self.env.step(dict(action="MoveAhead", moveMagnitude=0.25))

Similar to AI2-THOR, this now allows for either a dict to be passed in (i.e., it's fully backwards compatible), or just the kwargs to be passed in, as in:

self.env.step(action="MoveAhead", moveMagnitude=0.25)

[lgtm-com[bot]]

This pull request introduces 8 alerts when merging 860f1d43bd21306053082e7a3b6299a24550f2e0 into 0ea4c02152c79e554d7ef3f5e45cffca81be8f4f - view on LGTM.com

new alerts:

• 8 for Wrong number of arguments in a call

[Lucaweihs]

As the LGTM.com checks call out: this change would break lots of things. You should instead do something something like

def step(self, action_dict: Optional[Dict[str, Union[str, int, float]]] = None, **kwargs: Dict[str, Union[str, int, float]]):
    if action_dict is None:
        action_dict = {}
    action_dict.update(kwargs)
    ...

if you want this functionality.

[mattdeitke]

Hmm.. Interesting.. Ok. I thought I recalled that passing in a dict to **kwargs would work, but perhaps that's only if one uses kwargs=dict(...).

[mattdeitke]

Edit: ah wait, types are now off though. Will change back.

---

Updated and tested in: https://colab.research.google.com/drive/1bKe9uGeJuXkVPhR3kMIJlWme9KnLjG5d?usp=sharing

Note that

def step(
    self,
    action_dict: Optional[Dict[str, Union[str, int, float]]] = None,
    **kwargs: Dict[str, Union[str, int, float]]
):
    if action_dict is None:
        action_dict = {}
    action_dict.update(kwargs)

was simplified to the equivalent

def step(
    self,
    action_dict: Optional[Dict[str, Union[str, int, float]]] = dict(),
    **kwargs: Dict[str, Union[str, int, float]]
):
    action_dict.update(kwargs)

[Lucaweihs]

I know you're reverted things but, as this is a common python gotcha, I should stress that doing

    action_dict: Optional[Dict[str, Union[str, int, float]]] = dict(),

can lead to absolutely disastrous consequences and is not at all equivalent to doing

    action_dict: Optional[Dict[str, Union[str, int, float]]] = None,

To see why this is the case, try running the following:

def test(yar = dict()):
    yar["a"] = yar.get("a", 0) + 1
    print(yar)

for i in range(10):
    test()

What you'll see is that the yar dictionary is persistent across calls to test!

[mattdeitke]

Oh, wow. Interesting. I imagined yar = dict() created a new dict instance each time test() was called.

[Lucaweihs]

@mattdeitke I'm happy with this being merged once you've addressed Jordi's type note above.

[mattdeitke]

Fixed. Also for consistency, in https://github.com/allenai/allenact/pull/290/commits/272d3e86ea85e1d98e1094106a5a52c1860d003f I've added a dict as a possible value for action_dict, since some actions like

controller.step(action="Teleport", rotation=dict(x=0, y=0, z=0))

take a dict as a value.