New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RLlib] For new API stack: Move SampleBatch column names (e.g. SampleBatch.OBS) into new class (Columns
).
#43665
[RLlib] For new API stack: Move SampleBatch column names (e.g. SampleBatch.OBS) into new class (Columns
).
#43665
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Does this mean SampleBatch
will be deprecated soon and instead we run with dict
s or should the learner in the future run on episodes?
@@ -37,6 +37,7 @@ | |||
from ray.rllib.algorithms.algorithm_config import AlgorithmConfig | |||
from ray.rllib.algorithms.registry import ALGORITHMS_CLASS_TO_NAME as ALL_ALGORITHMS | |||
from ray.rllib.connectors.agent.obs_preproc import ObsPreprocessorConnector | |||
from ray.rllib.core.columns import Columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already the PR title made my heart beat faster :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha!
@@ -241,7 +241,7 @@ def additional_update_for_module( | |||
@OverrideToImplementCustomLogic | |||
def _compute_values( | |||
self, | |||
batch_for_vf: Union[MultiAgentBatch, NestedDict], | |||
batch_for_vf: Dict[str, Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we swtich now to the plain dictionary. I remember the ActorCriticEncoder
uses indeed a two level NestedDict
- is this also planned for these classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but yes, I would like to simplify things into using pure dicts.
This will be a separate PR, in which I'll try for the new API stack to not depend on these classes anymore: SampleBatch, MultiAgentBatch, NestedDict.
Signed-off-by: Sven Mika <svenmika1977@gmail.com>
…_sample_batch_fields_to_data Signed-off-by: sven1977 <svenmika1977@gmail.com> # Conflicts: # rllib/env/multi_agent_env_runner.py # rllib/env/single_agent_env_runner.py
…' into move_sample_batch_fields_to_data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just an add-on to the larger commits.
…_sample_batch_fields_to_data
…_sample_batch_fields_to_data
For new API stack: Move SampleBatch column names (e.g. SampleBatch.OBS) into new class (
Columns
).Background: We are trying to not rely anymore on SampleBatch or MultiAgentBatch in the new API stack (even though migration away from these has not been completed yet). Instead, we are aiming at simply using python dicts.
Same is true for NestedDicts.
Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.