Group tensorboard metrics

[VictorSanh]

As discussed in issue #38:

• I grouped the metrics into a few groups
• Added speed metrics (under iteration-time)
• Added the mapping iteration (or step) vs samples

A preview of the tensorboard on a dummy training can be found here.

[stas00]

Thank you for implementing this, @VictorSanh

I proposed to use f-strings consistently.

And also a few small questions.

[stas00]

btw, if you go to "Files Changed" tab, you can merge multiple suggestions in one click, so you then need to click half as much ;)

[VictorSanh]

btw, if you go to "Files Changed" tab, you can merge multiple suggestions in one click, so you then need to click half as much ;)

ooooh, wasn't aware of that, learned something!

[VictorSanh]

merging now, thank you @stas00 for the review!

[stas00]

BTW, that's not what I meant. Perhaps let's give a bit of time to have a chance to review changes in the future before merging.

# bug fix in xyz comments are great inside tests since it's then self-explanatory of what the test is testing.

Such comments in the code have no added value and only make the code harder to read. All of software is ridden with bugs, the normal (as compared to tests) code comments are useful when they explain why something is done when it's not obvious from reading the code.

So when I made that comment I was proposing to add a comment why it was is_last_rank(), if it wasn't obvious from the code, which it is not, which you explained in your Issue report to Megatron. This is what we want in the code. Currently the way it's committed the user has to go and research why. Whereas the only comment needed there was:

# only the last rank process has a non-None _GLOBAL_TENSORBOARD_WRITER

Hope my logic makes sense.

[VictorSanh]

Thanks for flagging that @stas00, always good to write better comments!
I fixed that with your suggestion (5e3963d).

I initially merged since you approved the changes. Usually, I use "Request changes" when things are bugging me and "Approved" as a final signal that the PR can be merged when things have been fixed. Does this make any sense to you?

[stas00]

Thank you for fixing the comment, Victor

This is the workflow pattern we use at transformers, a general Approval is made when things are more or less correct and then the nits can take some time to sort out. This always works when the PR writer is not a committer too, since they can't merge. When it's between us the committers we usually check in again when we feel it's ready to be merged.

We can try a different pattern here, and not to Approve until it's ready to be merged. I think this is a less ambiguous communication style. Over at transformers an early Approval I think helps the contributor to know they are "out of the woods" and the finish line is near.