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] Fix MetricsLogger race conditions. #49888

Merged
merged 2 commits into from
Jan 17, 2025
Merged

[RLlib] Fix MetricsLogger race conditions. #49888

merged 2 commits into from
Jan 17, 2025
+42 −33

Conversation

sven1977
Copy link
Contributor

@sven1977 sven1977 commented Jan 16, 2025 •
edited
Loading

Fix MetricsLogger race conditions.

  • Add more with self._threading_lock blocks around critical code that alters the self.stats dict.

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

Sorry, something went wrong.

@sven1977
wip

Verified

This commit was signed with the committer’s verified signature.
MarcoPolo Marco Munizaga
GPG key ID: E747E431D981A953
Verified
Learn about vigilant mode
Loading
Loading status checks…
94c4dd5 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
simonsays1980
simonsays1980 approved these changes Jan 16, 2025
View reviewed changes
Copy link
Collaborator

@simonsays1980 simonsays1980 left a comment

Choose a reason for hiding this comment

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

LGTM.

@sven1977
wip

Verified

This commit was signed with the committer’s verified signature.
MarcoPolo Marco Munizaga
GPG key ID: E747E431D981A953
Verified
Learn about vigilant mode
14f996e 
Signed-off-by: sven1977 <svenmika1977@gmail.com>
@sven1977 sven1977 enabled auto-merge (squash) January 17, 2025 09:11
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Jan 17, 2025
@sven1977 sven1977 merged commit 0ca5b57 into ray-project:master Jan 17, 2025
6 of 7 checks passed
@sven1977 sven1977 deleted the fix_metrics_logger_make_thread_safe branch January 17, 2025 16:50
ruisearch42 pushed a commit to ruisearch42/ray that referenced this pull request Jan 17, 2025
@sven1977 @ruisearch42
[RLlib] Fix MetricsLogger race conditions. (ray-project#49888)
b1e07e3
@sven1977 sven1977 added rllib RLlib related issues rllib-newstack labels Jan 20, 2025
anson627 pushed a commit to anson627/ray that referenced this pull request Jan 31, 2025
@sven1977 @anson627
[RLlib] Fix MetricsLogger race conditions. (ray-project#49888)
f9bf643 
Signed-off-by: Anson Qian <anson627@gmail.com>
srinathk10 pushed a commit that referenced this pull request Feb 2, 2025
@sven1977 @srinathk10
[RLlib] Fix MetricsLogger race conditions. (#49888)
ba25e17
anyadontfly pushed a commit to anyadontfly/ray that referenced this pull request Feb 13, 2025
@sven1977 @anyadontfly
[RLlib] Fix MetricsLogger race conditions. (ray-project#49888)
83ecf23 
Signed-off-by: Puyuan Yao <williamyao034@gmail.com>
park12sj pushed a commit to park12sj/ray that referenced this pull request Mar 18, 2025
@sven1977 @park12sj
[RLlib] Fix MetricsLogger race conditions. (ray-project#49888)
38bb588
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonsays1980 simonsays1980

Assignees
No one assigned
Labels
go add ONLY when ready to merge, run all tests rllib RLlib related issues rllib-newstack
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sven1977 @simonsays1980