Skip to content

Conversation

@finbarrtimbers
Copy link
Collaborator

@finbarrtimbers finbarrtimbers commented Sep 23, 2025

Previously, we only timed the entire weight sync event, and not the individual ones. This adds timing for each one.

To enable this, we now record timing of each future in ray_get_with_progress.

@finbarrtimbers finbarrtimbers marked this pull request as ready for review September 23, 2025 23:20
Copy link
Contributor

@mnoukhov mnoukhov left a comment

Choose a reason for hiding this comment

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

lgtm, suggestion is optional

Comment on lines +2189 to +2197
# Calculate distribution statistics
sync_time_stats = {
"time/weight_sync": timer.duration,
"time/weight_sync_mean": np.mean(actor_sync_times),
"time/weight_sync_min": np.min(actor_sync_times),
"time/weight_sync_max": np.max(actor_sync_times),
"time/weight_sync_median": np.median(actor_sync_times),
}

Copy link
Contributor

Choose a reason for hiding this comment

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

you could consider keeping the actual list and logging a histogram

@finbarrtimbers finbarrtimbers added this pull request to the merge queue Sep 25, 2025
Merged via the queue into main with commit 045cc0a Sep 25, 2025
3 checks passed
@finbarrtimbers finbarrtimbers deleted the time-weight-syncs branch October 10, 2025 20:36
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.

2 participants