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] Learner API: Fix and unify grad-clipping configs and behaviors. #34464

Merged
merged 9 commits into from
Apr 27, 2023
Merged

[RLlib] Learner API: Fix and unify grad-clipping configs and behaviors. #34464

merged 9 commits into from
Apr 27, 2023

Conversation

sven1977
Copy link
Contributor

@sven1977 sven1977 commented Apr 17, 2023 •
edited
Loading

Learner API: Fix and unify grad-clipping configs and behaviors.

  • Introduce new AlgorithmConfig setting: grad_clip_by, which can be set to "value", "norm" or "global_norm" and determines the mode of clipping.
  • Made grad_clip a generic AlgorithmConfig property (was only supported by some algos before). However, this setting is only used if _enable_learner_api=True.
  • Implement proper clipping behaviors based on the 3 new modes in Tf- and TorchLearner (postprocess_gradients method).
  • Add test cases for these new behaviors.

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
wip
dc34c33 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
6059b3e 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
38e2a0d 
Signed-off-by: sven1977 <[email protected]>
@sven1977
fixes and test case
9e7daf0 
Signed-off-by: sven1977 <[email protected]>
avnishn
avnishn reviewed Apr 18, 2023
View reviewed changes
Copy link
Member

@avnishn avnishn left a comment

Choose a reason for hiding this comment

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

Thanks for sharing this. However, I am overall confused by this pr. Is it incomplete?

rllib/core/learner/tf/tf_learner.py Outdated
) for k, v in gradients_dict.items()
}

# Clip by L2-norm (per gradient tensor).
Copy link
Member

Choose a reason for hiding this comment

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

are we going to allow users to clip gradients by value and then by norm? that doesn't sound right to me, but that is the behavior that has been enabled here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. Was thinking about this myself. Maybe we should just do: grad_clip: [some value] and then grad_clip_by: [value|norm|global_norm].

rllib/algorithms/qmix/qmix.py Outdated

# TODO (sven): Deprecate grad_clip setting once all-in on new Learner API.
self.grad_clip = 10.0
self.grad_clip_by_global_norm = 10.0
Copy link
Member

Choose a reason for hiding this comment

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

how is this going to be used downstream by the qmix policy / multi_gpu_train_one_batch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed by replacing the new settings with backward-compatible ones

@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into post…
5090ed0 
…process_gradients_implementations
@sven1977
wip
6e53eb4 
Signed-off-by: sven1977 <[email protected]>
@sven1977
wip
149190e 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Merge branch 'master' of https://github.com/ray-project/ray into post…
1afbf7f 
…process_gradients_implementations
@sven1977
wip
62ec14d 
Signed-off-by: sven1977 <[email protected]>
@sven1977
Copy link
Contributor Author

Hey @avnishn , sorry about the confusion and thanks for taking a look!

I went through the PR once more and your comments and addressed all of them.
I changed the configs to the backward-compatible pair of grad_clip and (new) grad_clip_by, which specifies the mode of clipping. This way, it's a) backward compatible and b) exclusive (only one way of clipping at the same time allowed).

sven1977
sven1977 commented Apr 26, 2023
View reviewed changes
rllib/algorithms/impala/impala.py
@@ -422,16 +425,6 @@ def validate(self) -> None:
self.vtrace_clip_pg_rho_threshold
)

@override(AlgorithmConfig)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed anymore with this PR. grad clipping has been universally moved into Learner.postprocess_gradients().

sven1977
sven1977 commented Apr 26, 2023
View reviewed changes
rllib/core/learner/torch/torch_learner.py
@@ -79,6 +79,22 @@ def compute_gradients(

return grads

@override(Learner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was missing in torch thus far.

sven1977
sven1977 commented Apr 26, 2023
View reviewed changes
rllib/utils/tf_utils.py
@@ -27,6 +27,48 @@
tf1, tf, tfv = try_import_tf()


@PublicAPI
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New grad clip utilities, replacing the old, messy ones (some of the old ones do clipping by value, some by norm (not global norm), some have additional optimizer update logic in them, etc..

@sven1977 sven1977 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Apr 26, 2023
@sven1977 sven1977 merged commit 25a5bcb into ray-project:master Apr 27, 2023
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@sven1977 @ProjectsByJackHe
[RLlib] Learner API: Fix and unify grad-clipping configs and behavior…
2ea2028 
…s. (ray-project#34464)

Signed-off-by: Jack He <[email protected]>
architkulkarni pushed a commit to architkulkarni/ray that referenced this pull request May 16, 2023
@sven1977 @architkulkarni
[RLlib] Learner API: Fix and unify grad-clipping configs and behavior…
2024a06 
…s. (ray-project#34464)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avnishn avnishn avnishn approved these changes

@gjoliver gjoliver Awaiting requested review from gjoliver

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

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@kouroshHakha kouroshHakha Awaiting requested review from kouroshHakha

@krfricke krfricke Awaiting requested review from krfricke

Assignees

@avnishn avnishn

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.

None yet
2 participants
@sven1977 @avnishn