transformers icon indicating copy to clipboard operation
transformers copied to clipboard

Add rounding error check to _maybe_log_save_evaluate

Open marcndo opened this issue 8 months ago • 7 comments

What does this PR do?

This PR aims to address the issue raised in #38032 by deciding the format in which the loss should be logged

Fixes # (issue) #38032

Before submitting

  • [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • [ ] Did you read the [contributor guideline] (https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#create-a-pull-request), Pull Request section? Yes
  • [ ] Was this discussed/approved via a Github issue or the forum? Please add a link to it if that's the case. https://github.com/huggingface/transformers/issues/38032#issuecomment-2953713166
  • [ ] Did you make sure to update the documentation with your changes? Here are the documentation guidelines, and here are tips on formatting docstrings. Yes
  • [ ] Did you write any new necessary tests? No

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag members/contributors who may be interested in your PR.

marcndo avatar Jun 10 '25 03:06 marcndo

Hi @marcndo, there are a lot of unrelated changes in this PR! Can you make sure you rebase to the latest commit and try to limit the changes to just the relevant loss rounding lines?

Rocketknight1 avatar Jun 10 '25 12:06 Rocketknight1

Sure, I'll proceed with this right away

marcndo avatar Jun 10 '25 20:06 marcndo

Looks okay to me now! cc @sunmarc

Rocketknight1 avatar Jun 11 '25 12:06 Rocketknight1

Certainly, I will proceed with this.

marcndo avatar Jun 12 '25 16:06 marcndo

I tried the logging with wandb and it pointed out an issue logs["loss"] = str(round(log_eval), 4) instead of logs["loss"] = str(round(log_eval, 4)). After this fix the logging seems to work as expected here attached is a screenshot. I guess i should update the PR with this new fix. Screen Shot 2025-06-13 at 2 42 15 AM

marcndo avatar Jun 13 '25 01:06 marcndo

I mean can the values be correctly displayed on the wandb table/website ? Also seems like there are some issues with changing it to an str. Another simple idea would be to just round it to 10 like logs["loss"] = round(log_eval, 10 when x < 1e-4, that should be enough precision

SunMarc avatar Jun 13 '25 15:06 SunMarc

I'm not sure how to verify if the values are correctly displayed on the wandb table/website. Could you provide any guidance on how to confirm this, please?

marcndo avatar Jun 13 '25 18:06 marcndo

Just use trainer with wandb installed, it will report the results to their website

SunMarc avatar Jun 16 '25 12:06 SunMarc

Screen Shot 2025-06-17 at 11 48 02 PM

marcndo avatar Jun 18 '25 16:06 marcndo

Hi @SunMarc, please let me know if any modification is needed on this PR.

marcndo avatar Jun 18 '25 19:06 marcndo

Hi @SunMarc, just wanted to kindly follow up on the PR to see if everything looks good now or if any further changes are needed. Thanks!

marcndo avatar Jun 23 '25 12:06 marcndo