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 some deprecation messages (they shouldn't be there) and make others error=True (from error=False) #38555

Merged
merged 8 commits into from
Aug 28, 2023
Merged

[RLlib] Clean up some deprecation messages (they shouldn't be there) and make others error=True (from error=False) #38555

merged 8 commits into from
Aug 28, 2023

Conversation

sven1977
Copy link
Contributor

@sven1977 sven1977 commented Aug 17, 2023 •
edited
Loading

Clean up some deprecation messages (they shouldn't be there) and make others error=True (from error=False)

Why are these changes needed?

Related issue number

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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

@sven1977
fixes
803a0f1 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
@sven1977
wip
0efdc0c 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into dele…
e16e495 
…te_deprecation_messages_for_old_stack
@sven1977
fixes
4c14bbc 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
ArturNiederfahrenhorst
ArturNiederfahrenhorst reviewed Aug 21, 2023
View reviewed changes
rllib/BUILD Outdated
# size = "medium",
# srcs = ["examples/batch_norm_model.py"],
# args = ["--as-test", "--framework=tf", "--run=PPO", "--stop-reward=80"]
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave a comment here on why this is commented out and when it should be reenabled, please?
Also, depending on the reason, it might become a "new stack issue"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reestablished test case. It's ok to leave our old tf register_variables NOT deprecated for now.

ArturNiederfahrenhorst
ArturNiederfahrenhorst reviewed Aug 21, 2023
View reviewed changes
rllib/models/base_model.py
@@ -207,7 +207,6 @@ def _update_outputs_and_next_state(
return outputs, next_state


@Deprecated(error=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing this deprecation warning? This is going to be deprecated soon according to our plans, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely. However, it is still active in many algos and thus users receive deprecation warnings for using non-deprecated algos. Also most algos (except PPO) still use the _enable_learner_api=False by default, so these warnings are too confusing.

ArturNiederfahrenhorst
ArturNiederfahrenhorst reviewed Aug 21, 2023
View reviewed changes
rllib/models/catalog.py
@@ -202,7 +201,6 @@
# fmt: on


@Deprecated(old="rllib.models.catalog.ModelCatalog", error=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, why are we removing this? Simply too verbose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, too verbose and confusing.

ArturNiederfahrenhorst
ArturNiederfahrenhorst reviewed Aug 21, 2023
View reviewed changes
Copy link
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst left a comment

Choose a reason for hiding this comment

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

Cool cleanup. Thanks! Just one nit on the commented out ppo test and I'd like to know why we are moving all of this stuff we want to deprecate from warnings to developer API.

kouroshHakha
kouroshHakha reviewed Aug 22, 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.

Does this PR solve the deprecation warning regarding exploration setting being deprecated? If so, I couldn't find the code that deals with this.

@sven1977
fixes
df03eb3 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
@sven1977
Copy link
Contributor Author

Hey @kouroshHakha , the deprecation warnings for all exploration classes had already been fixed in an earlier PR.

@sven1977
fixes and LINT
92a801b 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
@sven1977
Copy link
Contributor Author

Hey @ArturNiederfahrenhorst , we are moving "back to" DeveloperAPI b/c these classes/functions must - per our LINTer - have some API tag.

@sven1977
wip
3034a3a 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into dele…
9ea0731 
…te_deprecation_messages_for_old_stack
@sven1977 sven1977 merged commit 0f62ccc into ray-project:master Aug 28, 2023
37 of 40 checks passed
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
@sven1977 @arvind-chandra
[RLlib] Clean up some deprecation messages (they shouldn't be there) …
27848a9 
…and make others `error=True` (from `error=False`) (ray-project#38555)

Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
LeonLuttenberger pushed a commit to jaidisido/ray that referenced this pull request Sep 5, 2023
@sven1977 @LeonLuttenberger
[RLlib] Clean up some deprecation messages (they shouldn't be there) …
bc506b7 
…and make others `error=True` (from `error=False`) (ray-project#38555)
jimthompson5802 pushed a commit to jimthompson5802/ray that referenced this pull request Sep 12, 2023
@sven1977 @jimthompson5802
[RLlib] Clean up some deprecation messages (they shouldn't be there) …
15eb91f 
…and make others `error=True` (from `error=False`) (ray-project#38555)

Signed-off-by: Jim Thompson <jimthompson5802@gmail.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
@sven1977
[RLlib] Clean up some deprecation messages (they shouldn't be there) …
301079c 
…and make others `error=True` (from `error=False`) (ray-project#38555)

Signed-off-by: Victor <vctr.y.m@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArturNiederfahrenhorst ArturNiederfahrenhorst ArturNiederfahrenhorst left review comments

@kouroshHakha kouroshHakha kouroshHakha approved these changes

@gjoliver gjoliver Awaiting requested review from gjoliver

@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

@ArturNiederfahrenhorst ArturNiederfahrenhorst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sven1977 @ArturNiederfahrenhorst @kouroshHakha