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] Add backward compatibility to MeanStdFilter to restore from older checkpoints. #30439

Conversation

simonsays1980
Copy link
Collaborator

@simonsays1980 simonsays1980 commented Nov 17, 2022

…heckpoints.

Why are these changes needed?

Restoring in Ray 2.0.1 checkpoints from older versions where MeanStdFilter was used for training results in an error:

AttributeError: other has no attribute named 'running_stats'

The same goes for self.mean_array and self.std_array simply because these attributes were named differently.

This PR adds some safeguarding for backwards compatibility. I tested this with my own checkpoints from Ray 2.0.0 and it works.

Related issue number

None. Faught with it, saw it, fixed it.

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 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 :(

@simonsays1980 simonsays1980 force-pushed the meanstdfilter-backward-compatibility branch from bece744 to eb362bc Compare November 17, 2022 22:35
@ArturNiederfahrenhorst ArturNiederfahrenhorst self-assigned this Dec 1, 2022
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.

Thanks for this contribution @simonsays1980 ! I actually changed the names here to make the code well readable but forgot about the compatibility.

)
other.std_array = (
np.copy(self.std_array) if hasattr(self, "std_array") else np.copy(self._S)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, it was #27864 that made these changes. All changed attribute names seem to be covered here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: A TODO that tells us to remove this additional code at some point. But it does not do any harm here either 😃

Copy link
Collaborator Author

@simonsays1980 simonsays1980 Dec 2, 2022

Choose a reason for hiding this comment

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

I can add the nit :D

I also saw that we get often a DeprecationWarning with this class which could be overcome probably by choosing () instead of None in the shape arguments of numpy array ctors.

I could push that together into one smooth PR if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate PRs for separate problems would be appreciated!

…ged 'shape=None' for 'shape=()' as interchangeability of 'None' and '()' will be deprecated and only '()' will stay.

Signed-off-by: Simon Zehnder <[email protected]>
@stale
Copy link

stale bot commented Jan 2, 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 2, 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 2, 2023
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! Thanks for the fix @simonsays1980 ! :)

@sven1977 sven1977 changed the title Added backward compatibility to MeanStdFilter to restore from older c… [RLlib] Add backward compatibility to MeanStdFilter to restore from older checkpoints. Jan 3, 2023
@sven1977 sven1977 merged commit 256867b into ray-project:master Jan 3, 2023
AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023
tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023
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