Fixed reporting of single value of loss and ppl across devices. #496
Conversation
quic-meetkuma
commented
Jul 7, 2025
quic-meetkuma
commented
- Fixed reporting of single value of loss and ppl across devices.
- Minor refactoring changes and variable name changes to make it consistent.
Signed-off-by: meetkuma <[email protected]>
|
|
||
| if local_rank == 0: |
There was a problem hiding this comment.
local_rank will be None in non ddp case. Hence, it will not update the tensorboard in non DDP case. Defining following method in helper.py and calling it over here will help:
def is_rank_zero():
return int(os.getenv("LOCAL_RANK", 0)) == 0
There was a problem hiding this comment.
Good catch, will update!
| # Update the learning rate as needed | ||
| lr_scheduler.step() | ||
|
|
||
| if train_config.run_validation: | ||
| if train_config.enable_ddp: | ||
| dist.barrier() |
There was a problem hiding this comment.
It is code refactoring. Moved inside evaluation function.
| eval_epoch_loss = ( | ||
| 0.0 if eval_loss == 0.0 else eval_loss / (step + 1 - num_dummy_samples / train_config.val_batch_size) | ||
| ) | ||
| eval_loss = 0.0 if eval_loss == 0.0 else eval_loss / (step + 1 - num_dummy_samples / train_config.val_batch_size) |
There was a problem hiding this comment.
Since we are using the variable name train_epoch_loss for the average train loss of the epoch, it will be good to keep the name eval_epoch_loss for the average evaluation loss of the epoch to maintain uniformity.
There was a problem hiding this comment.
Check the other variables being returned from this function. Made the names consistent.
Signed-off-by: meetkuma <[email protected]>
quic-meetkuma
requested review from
quic-rishinr,
ochougul,
quic-hemagnih and
quic-amitraj
as code owners
| dist.all_reduce(eval_loss, op=dist.ReduceOp.SUM) | ||
| eval_loss /= get_num_ddp_devices() | ||
| dist.all_reduce(eval_metric, op=dist.ReduceOp.SUM) | ||
| eval_metric /= get_num_ddp_devices() |
There was a problem hiding this comment.
will it not make each json files have same data for train_epoch_loss, train_epoch_metric and val_epoch_loss and val_epoch_metric?
There was a problem hiding this comment.
train_epoch_loss and train_epoch_metric are all_reduced after they are populated at L357.
For this eval_loss and eval_metric, you are right. I will move this all reduce after L386.
