$\color{#D29922}\textsf{\Large⚠\kern{0.2cm}\normalsize Warning}$ Result of CPU Benchmark Tests

Total Benchmarks: 89. Improved: $\large\color{#35bf28}7$. Worsened: $\large\color{#d91a1a}5$.

This is awesome, I think it might also be helpful for #1201 since Unity supports masked actions as well.

I also think this is awsome. It will be needed for MLAgents, as @hyerra said, for PettingZoo #1441 and for SMAC #1466

A few things to be aware of:

• this works well when you have to mask some of the actions but some are still available. how do we treat the case when all actions are unavailable (eg an agent dies)?
• check_env_specs needs also to be adapted, because it will try to check that the last action is in the current action spec, but the mask of the current action spec has already been updated after the last step.

A few things to be aware of:

• this works well when you have to mask some of the actions but some are still available. how do we treat the case when all actions are unavailable (eg an agent dies)?

Shouldn't it be done then?
If we think about other situations, I'm not sure it makes sense to cover this explicitly. Example: if you're playing a game and you have N actions available but when your turn comes you don't have anything to do, there should be one more action N+1 "pass".

Example: if you're playing a game and you have N actions available but when your turn comes you don't have anything to do, there should be one more action N+1 "pass".

I m talking about a different scenario.
For example, you are playing a game, but only one player acts at a time. It is not your turn, but you are also not done.

What i am doing right now is that i am setting a mask of all false for all actions of that agent, but not passing that to the spec (as it would crash), just having it in the env output. So the spec will still see all actions available for that agent, but in reality that whole transition for it should be masked out by the user later.

is there a better way to treat this?
Maybe passing a mask of all false should be allowed? And all specs should have mask compatibility?

Maybe passing a mask of all false should be allowed? And all specs should have mask compatibility?

Then what should action_spec.rand() return? It's gonna be confusing no?
IMO if your env has the option of having an action "do nothing" this is something that should be in the env otherwise it will be surprising.
In this case, we can do a transform:

class SkipAction(Transform):
    """Adds an action to the env that skips the step if selected.
    """

For instance, if action_spec has 6 possible actions, an action with value 6 will instruct the transform to skip the step.
A bit of magic will be needed to make it work with composite actions...

ok yeah I see.
we can keep this PR to the current scope then

Also we need to add this line to check_env_specs, because otherwise the check will fail when the action_mask gets updated

# Check specs
last_td = real_tensordict[..., -1]
# replace action with one sampled from last spec
# (since spec mask might be changed after last step)
last_td = env.rand_action(last_td) ## <------ this line
full_action_spec = env.input_spec["full_action_spec"]
full_state_spec = env.input_spec["full_state_spec"]

LGTM

are we leaving the multi spec for another PR?

you mean multi-one-hot (ie in a single tensor) or multiple masks?

Multi one hot and multi discrete

@matteobettini now these are supported too