[BugFix] QValue modules and nested action #1351
Conversation
Signed-off-by: Matteo Bettini <matbet@meta.com>
facebook-github-bot
added
the
CLA Signed
| if "action" in spec_keys: | ||
| _key = "action" | ||
| else: | ||
| # the first key is the action |
There was a problem hiding this comment.
If we do not find an action entry, we pick the first leaf (as done in EnvBase to get an action_key)
There was a problem hiding this comment.
Do you think it makes sense? Do we want to be more restrictive?
There was a problem hiding this comment.
We could only accept key that end with "action" as leaf.
up to you, i maybe slightly prefer the current version
There was a problem hiding this comment.
I have modified it so that it will look for a leaf "action" key
| spec_keys = action_space.keys(True, True) | ||
| if "action" in spec_keys: | ||
| _key = "action" | ||
| else: |
|
So @vmoens I have a proposition here. This all business of allowing both What about having If so a user which wants to pass a str can pass it to Currently we allow to feed a spec to either |
We can make the change but unfortunately as of now I'm not open to bc breaking changes without warning, ie if we do that we must ensure that users will only see a warning until 0.3 (since this will be in 0.2). |
|
Sure, I can put the warning |
Signed-off-by: Matteo Bettini <matbet@meta.com>
matteobettini
force-pushed
the
parametric_modules
branch
from
949340c to
2a196fd
Compare
Signed-off-by: Matteo Bettini <matbet@meta.com>
| if "action" in spec_keys: | ||
| _key = "action" | ||
| else: | ||
| # the first key is the action |
There was a problem hiding this comment.
Do you think it makes sense? Do we want to be more restrictive?
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Fixes #1273