-
Notifications
You must be signed in to change notification settings - Fork 603
add memory metrics to TensorBoard #60
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
Conversation
[ghstack-poisoned]
train.py
Outdated
| "global_avg_loss": global_avg_loss, | ||
| "global_max_loss": global_max_loss, | ||
| "loss/global_avg": global_avg_loss, | ||
| "loss/global_max": global_max_loss, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - using the / here is confusing to me...I thought it represented the loss divided by the global avg, and same for max...
maybe consider just an _ or : or even :: as the separator? (loss:global_avg, loss::global_max, memory_current_active).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good point. I use [tag]/[metric] here because TB collects plots under the same [tag] together in a row, so that they form a visual group. Just like in the picture in PR summary, memory metrics are grouped into memory_current, and memory_peak. I'll explore a way that can achieve this but without ambiguity for losses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some exploration, e.g. tried to put related metrics into a single plot. The options we have are add_scalars and add_custom_scalars, and it seems neither is ideal (e.g.). I'm changing loss/global_avg to loss_metrics/global_avg for now to make it less ambiguous.
lessw2020
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, thanks for integrating these stats!
one very minor nit about the / being possibly confused as division when used in labelling.
<img width="1391" alt="Screenshot 2024-02-15 at 5 19 09 PM" src="https://github.com/pytorch-labs/torchtrain/assets/150487191/af8a2efb-13ff-4e8f-84f2-b245784747ed"> [ghstack-poisoned]
ghstack-source-id: da7e02b Pull Request resolved: pytorch#60
Stack from ghstack (oldest at bottom):