[RLlib; Tune] Fix WandB metric overlap after restore from checkpoint. #46897
+5
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: sven1977 <[email protected]>
…wandb_metric_overlap_after_restore_from_checkpoint
Signed-off-by: sven1977 <[email protected]>
sven1977
requested review from
ArturNiederfahrenhorst and
simonsays1980
as code owners
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
sven1977
added
the
tests-ok
justinvyu
approved these changes
Aug 1, 2024
There was a problem hiding this comment.
Makes sense to me!
By the way, this will only fix the issue for the setup_wandb flow where users call that directly in their code. This does not solve the WandbLoggerCallback case -- should we fix it there too?
Nevermind. this is the exact line of code that was changed.
|
Yeah, that's what I thought, too. Thanks for clarifying and for your review @justinvyu ! :) |
sven1977
changed the title
simonsays1980
approved these changes
Aug 2, 2024
| @@ -15,6 +15,7 @@ | |||
| from ray._private.storage import _load_class | |||
| from ray.air import session | |||
| from ray.air._internal import usage as air_usage | |||
| from ray.air.constants import TRAINING_ITERATION | |||
There was a problem hiding this comment.
If air goes anyways shouldn't this class maybe move over to train or tune?
| @@ -415,7 +416,7 @@ def run(self): | |||
| log, config_update = self._handle_result(item_content) | |||
| try: | |||
| self._wandb.config.update(config_update, allow_val_change=True) | |||
| self._wandb.log(log) | |||
| self._wandb.log(log, step=log.get(TRAINING_ITERATION)) | |||
sven1977
deleted the
fix_wandb_metric_overlap_after_restore_from_checkpoint
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix WandB metric overlap after restore from checkpoint.
This PR fixes the following problem:
Tuner.fit(), checkpoints every 100 iterations, and uses the WandB integration.Tuner.resume()to resume it from the last checkpoint (which would have happened at iteration 100).To fix this, we'll make sure to always send the explicit
stepalong with eachwandb.log()call.Note that upon restoring from a checkpoint, RLlib already properly sets
Trainable.training_iterationto the value stored in the last checkpoint. OtherTrainablesubclasses might have to implement a similar logic in their checkpointing behavior or we'll eventually add this to the base Trainable class.Note also that this PR does NOT affect the behavior for users using the
setup_wandb()utility and then log WandB metrics manually (for example in their Ray Train training functions).Why are these changes needed?
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.