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] Fix Issue #25316: unconfigurable dist_dim for custom multi-action distributions #25490

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

[RLlib] Fix Issue #25316: unconfigurable dist_dim for custom multi-action distributions #25490

wants to merge 11 commits into from
+179 −172

Conversation

XuehaiPan
Copy link
Contributor

@XuehaiPan XuehaiPan commented Jun 5, 2022 •
edited
Loading

Why are these changes needed?

Make dist_dim for custom multi-action distributions configurable when the user subclassing MultiActionDistribution.

Changes:

  • Change ActionDistribution.required_model_output_shape from staticmethod to classmethod
  • Change Distribution.required_input_dim from staticmethod to classmethod
  • Extract some common methods for multi-distribution into a mixin class.

Related issues:

Related PR:

cc @sven1977

Related issue number

Fixes #25316

Checks

  • 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 :(
@XuehaiPan XuehaiPan force-pushed the fix-multi-action-dist branch 2 times, most recently from 4814ac7 to 132d15a Compare June 5, 2022 15:56
gjoliver
gjoliver reviewed Jun 21, 2022
View reviewed changes
rllib/models/action_dist.py


@DeveloperAPI
class MultiActionDistributionMixIn(ActionDistribution):
Copy link
Member

Choose a reason for hiding this comment

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

one thing I am curious is why does this have to be a MixIn, instead of having all this logic as part of MultiActionDistribution class?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one thing I am curious is why does this have to be a MixIn, instead of having all this logic as part of MultiActionDistribution class?

The point here is to reduce duplicate code. All these methods in MultiActionDistributionMixIn are shared for MultiActionDistribution for TensorFlow, PyTorch, and maybe Jax in the future.
@XuehaiPan XuehaiPan requested a review from a team as a code owner August 23, 2022 06:28
@gjoliver
Copy link
Member

@kouroshHakha may be the best person to review this PR once he is back from break.
@stale
Copy link

stale bot commented Sep 30, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.
@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Sep 30, 2022
@XuehaiPan
Copy link
Contributor Author

Any updates for this PR?
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Oct 3, 2022
@stale
Copy link

stale bot commented Jan 7, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.
@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jan 7, 2023
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jan 7, 2023
@XuehaiPan
format with black
a6e4dd4 
Signed-off-by: Xuehai Pan <XuehaiPan@pku.edu.cn>
@stale
Copy link

stale bot commented May 12, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.
@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label May 12, 2023
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label May 16, 2023
@XuehaiPan
Copy link
Contributor Author

A gentle ping for this. Any feedback is welcome. cc @sven1977 @avnishn @ArturNiederfahrenhorst
@stale
Copy link

stale bot commented Jun 17, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.
@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 17, 2023
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 18, 2023
@krfricke
Copy link
Contributor

I'll ping internally to get a review. Apologies for the delay.
@stale
Copy link

stale bot commented Oct 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.
@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Oct 15, 2023
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Oct 15, 2023
@stale Stale
Copy link

stale bot commented Dec 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.
@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Dec 15, 2023
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Dec 15, 2023
aslonnie
aslonnie reviewed Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

is this still relevant? should we close this?
@XuehaiPan
Copy link
Contributor Author

I think this is still relevant.
@anyscalesam anyscalesam added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale The issue is stale. It will be closed within 7 days unless there are further conversation
7 participants
@XuehaiPan @gjoliver @RocketRider @krfricke @aslonnie @kouroshHakha @anyscalesam