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.

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] Remove all default config objects and rllib/agents #33242

Merged

Conversation

ArturNiederfahrenhorst
Copy link
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst commented Mar 13, 2023

Why are these changes needed?

With Ray 2.0.0, we had made two migrations:
Trainer -> Algorithm
ConfigDicts -> AlgorithmConfig objects

This was over 8 months ago and appears to be a sufficient amount of time to fully deprecate the aliases for Trainers from the rllib/agent directory and the legacy config dicts _DEFAULT_CONFIG.

This is also necessary so that less config dicts trickle into RLlib to reduce errors that stem from us attemping to access these object's variables, while they still need to be indexed.

This PR is needed so that we can move forward with #29205

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
…missing configs

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
>>> ppo = ppo_config.build(env="Pendulum-v1")
>>> from ray.rllib.algorithms.ppo.ppo import PPOConfig # doctest: +SKIP
>>> ppo_config = PPOConfig.from_dict({...}) # doctest: +SKIP
>>> ppo = ppo_config.build(env="Pendulum-v1") # doctest: +SKIP
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 left this method here for now, since some folks might be actively using it still.
We can create deprecation warnings here if you will.


policy = Policy.from_checkpoint(path_to_checkpoint)
self.assertTrue(isinstance(policy, Policy))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can not support this anymore. Upholding this would require converting old config dicts inside of these checkpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood.

@@ -117,7 +117,7 @@ def test_nested_action_spaces(self):
ioctx.config["input_config"]["paths"], ioctx
)
config["input_config"] = {"paths": config["output"]}
del config["output"]
del config.output
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I don't think we should del this here, since it's now a property. Just set it to None?

Copy link
Contributor

@sven1977 sven1977 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 nit in my comments on del'ing a property.

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
@ArturNiederfahrenhorst ArturNiederfahrenhorst changed the title [RLlib] Remove all default config objects [RLlib] Remove all default config objects and rllib/agents Mar 15, 2023
@gjoliver gjoliver merged commit 8a9a176 into ray-project:master Mar 17, 2023
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request Mar 21, 2023
…ct#33242)

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
…ct#33242)

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
…ct#33242)

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
chaowanggg pushed a commit to chaowanggg/ray-dev that referenced this pull request Apr 4, 2023
…ct#33242)

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: chaowang <chaowang@anyscale.com>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
…ct#33242)

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
…ct#33242)

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants