Skip to content

Fix tokens_per_sec calculation in SpeedMonitor #415

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

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

konstantinjdobler
Copy link
Contributor

@konstantinjdobler konstantinjdobler commented Aug 15, 2023

The calculations for tokens_per_sec (and device/tokens_per_sec) in SpeedMonitor are wrong, specifically an overestimation by a factor of window_size (default to 100). To fix, we need to multiply samples_per_second by the mean number of tokens per sample (instead of multiplying by the sum of the number of tokens per sample over the last window_size batches, which is done currently).

For comparison, see the reference implementation:

# Compute token stats if dataloader.dataset has max_seq_len. Assumes no padding.
try:
    max_seq_len = state.dataloader.dataset.max_seq_len  # type: ignore
    # Only applicable to seq data / models
    logger.log_metrics({'throughput/tokens_per_sec': samples_per_sec * max_seq_len})
    logger.log_metrics({'throughput/device/tokens_per_sec': dev_samples_per_sec * max_seq_len})

https://github.com/mosaicml/composer/blob/f2a2dc820cb75023b9eb7c46fdfd25273712abd0/composer/callbacks/speed_monitor.py#L279-L284

Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

Thank you!

@carmocca carmocca merged commit 31037f9 into Lightning-AI:main Aug 15, 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.

2 participants