Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[RLlib] Clean up deprecated concat_samples calls #31391

Merged
merged 4 commits into from
Jan 5, 2023
Merged

[RLlib] Clean up deprecated concat_samples calls #31391

merged 4 commits into from
Jan 5, 2023

Conversation

ArturNiederfahrenhorst
Copy link
Contributor

Signed-off-by: Artur Niederfahrenhorst [email protected]

Why are these changes needed?

We are still using our own deprecated API for caoncating samples.

Screenshot 2023-01-02 at 18 17 41

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

kouroshHakha
kouroshHakha reviewed Jan 3, 2023
View reviewed changes
rllib/evaluation/sampler.py Outdated
@@ -92,7 +92,7 @@ def next(self) -> SampleBatchType:
batches = [self.get_data()]
batches.extend(self.get_extra_batches())
if len(batches) > 1:
Copy link
Contributor

@kouroshHakha kouroshHakha Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this if-else check anymore, Do you? What would concat_samples do if you have a list with only one item in it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concat batches does not know wether it is normally handling MA or SA batches. To avoid abscure errors where we simply pass an empty single agent batch down the line when the sampler samples no batches at all, I think we should raise an error here. There are currently no cases of this happening I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think that's a good idea.

kouroshHakha
kouroshHakha approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@kouroshHakha kouroshHakha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one semi-nit comment :)

rllib/utils/replay_buffers/multi_agent_mixin_replay_buffer.py Outdated
@@ -344,7 +344,7 @@ def check_buffer_is_ready(_policy_id):
if check_buffer_is_ready(policy_id):
samples.append(mix_batches(policy_id).as_multi_agent())

return MultiAgentBatch.concat_samples(samples)
return concat_samples(samples) if samples else MultiAgentBatch({}, 0)
Copy link
Contributor

@kouroshHakha kouroshHakha Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer writing this part like the following, it's much simpler and also explicit about the output being a MultiAgentBatch type.

concat_samples_into_ma_batch(samples)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you didn't get rid of MultiAgentBatch.concat_samples() entirely in file. I found one that is slightly above this line. Might be worth doing a search and replace? I wonder why this is not caught on the unittests if the error on deprecation is raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhhh that's weird. I'll check again, thanks! I'll follow the nits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked again and didn't find any occurences. But some occurences of concat_samples() which I turned into concat_samples_into_ma_batch()! Lmk if you can point to the MultiAgentBatch.concat_samples() calls

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyCharm also does not give me any results through search..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, wrong call. :)

kouroshHakha
kouroshHakha approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@kouroshHakha kouroshHakha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Plz merge once all the tests pass.

gjoliver
gjoliver approved these changes Jan 3, 2023
View reviewed changes
Copy link
Member

@gjoliver gjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much appreciated

@ArturNiederfahrenhorst
initial
d0b118e 
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
@ArturNiederfahrenhorst
fix mixinbuffer and lint
c0c826e 
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
@ArturNiederfahrenhorst
kourosh's nits
68aa1f1 
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
@ArturNiederfahrenhorst
remove ..concat_samples from a comment
497c5d0 
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
@ArturNiederfahrenhorst ArturNiederfahrenhorst force-pushed the deprecateconcat_samples branch 2 times, most recently from 3070c7e to 497c5d0 Compare January 4, 2023 22:53
@ArturNiederfahrenhorst
Copy link
Contributor Author

@gjoliver No idea why actor_manager tests kept failing. After rebasing three times they are green 🤔

@ArturNiederfahrenhorst ArturNiederfahrenhorst added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jan 5, 2023
@gjoliver gjoliver merged commit f44f578 into ray-project:master Jan 5, 2023
@ArturNiederfahrenhorst ArturNiederfahrenhorst deleted the deprecateconcat_samples branch January 5, 2023 15:39
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
@ArturNiederfahrenhorst @AmeerHajAli
[RLlib] Clean up deprecated concat_samples calls (#31391)
6fc1145 
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
@ArturNiederfahrenhorst @tamohannes
[RLlib] Clean up deprecated concat_samples calls (ray-project#31391)
790f124 
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: tmynn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kouroshHakha kouroshHakha kouroshHakha approved these changes

@gjoliver gjoliver gjoliver approved these changes

@sven1977 sven1977 Awaiting requested review from sven1977 sven1977 is a code owner

@avnishn avnishn Awaiting requested review from avnishn

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@krfricke krfricke Awaiting requested review from krfricke

Assignees

@kouroshHakha kouroshHakha

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ArturNiederfahrenhorst @gjoliver @kouroshHakha