[RLlib] Make Dataset reader default reader and enable CRR to use dataset #26304
Conversation
… into crr_use_worker_data_2
… into crr_use_worker_data_2
…hable_input_reader
… into crr_use_worker_data_2
… into crr_use_worker_data_2
…use_worker_data_2
avnishn
requested review from
sven1977,
gjoliver,
ArturNiederfahrenhorst,
smorad,
maxpumperla,
kouroshHakha and
krfricke
as code owners
There was a problem hiding this comment.
Thanks a lot @avnishn, this should unify our API a little bit. Though I have a few minor concerns before merging it. Thanks.
| if _setup: | ||
| # Force a local worker if num_workers == 0 (no remote workers). | ||
| # Otherwise, this WorkerSet would be empty. | ||
| self._local_worker = None | ||
| if num_workers == 0: | ||
| local_worker = True | ||
|
|
||
| if ( |
There was a problem hiding this comment.
yeah, why do we need to check all these additional things all of a sudden?
handling different input formats and shard splitting I remember is part of dataset_input.py?
| if num_workers: | ||
| self.batch_size = max(math.ceil(self.batch_size / num_workers), 1) | ||
| # We allow the creation of a non-functioning None DatasetReader. | ||
| # It's useful for example for a non-rollout local worker. | ||
| if ds: | ||
| if self._ioctx.worker is not None: | ||
| self._policy_map = self._ioctx.worker.policy_map | ||
| self._default_policy = self._policy_map.get(DEFAULT_POLICY_ID) |
There was a problem hiding this comment.
man, input_reader shouldn't have access to policy. this seems bad.
let's either find a different place for it, or make sure the data file has all the post-processed data bits in the first place, so we don't have to run it?
There was a problem hiding this comment.
the issue is that we need a way to support doing do trajectory post processing if needed and the only way to do that is if I have access to the policy.
This is how its done in the JSON reader I really just copied the logic. I would remove this, but needed to do this to pass tests. @gjoliver
There was a problem hiding this comment.
yeah I see why you are doing this. During sampling, env_runner calls this method and since now we are replacing env_runner basically with this input_reader we have to do something equivalent. It's a bad design for both actually. The post_processing trajectory should get called by default where the post_fn function is getting called right now. All these can be addressed with a proper callback design but we are far from that right now.
| if isinstance(batch, SampleBatch): | ||
| out = [] | ||
| for sub_batch in batch.split_by_episode(): | ||
| out.append(self._default_policy.postprocess_trajectory(sub_batch)) |
There was a problem hiding this comment.
I am going to think really hard so we don't have to do this here.
* master: (42 commits) [dashboard][2/2] Add endpoints to dashboard and dashboard_agent for liveness check of raylet and gcs (ray-project#26408) [Doc] Fix docs feedback button (ray-project#26402) [core][1/2] Improve liveness check in GCS (ray-project#26405) [RLlib] Checkpoint and restore connectors. (ray-project#26253) [Workflow] Minor refactoring of workflow exceptions (ray-project#26398) [workflow] Workflow queue (ray-project#24697) [RLlib] Minor simplification of code. (ray-project#26312) [AIR] Update TensorflowPredictor to new API (ray-project#26215) [RLlib] Make Dataset reader default reader and enable CRR to use dataset (ray-project#26304) [runtime_env] [doc] Remove outdated info about "isolated" environment (ray-project#26314) [Doc] Fix rate-the-docs plugin (ray-project#26384) [Docs] [Serve] Has a consistent landing page style (ray-project#26029) [dashboard] Add `RAY_CLUSTER_ACTIVITY_HOOK` to `/api/component_activities` (ray-project#26297) [tune] Use `Checkpoint.to_bytes()` for store_to_object (ray-project#25805) [tune] Fix `SyncerCallback` having a size limit (ray-project#26371) [air] Serialize additional files in dict checkpoints turned dir checkpoints (ray-project#26351) [Docs] Add "rate the docs" plugin for feedback on docs (ray-project#26330) [Doc] Fix actor example (ray-project#26381) Set RAY_USAGE_STATS_EXTRA_TAGS for release tests (ray-project#26366) [Datasets] Update docs for drop_columns and fix typos (ray-project#26317) ...
…set (ray-project#26304) Co-authored-by: avnish <avnish@avnishs-MBP.local.meter> Signed-off-by: Stefan van der Kleij <s.vanderkleij@viroteq.com>
Why are these changes needed?
Related issue number
Checks
scripts/format.shto lint the changes in this PR.