Skip to content

Add rounding error check to _maybe_log_save_evaluate #38714

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcndo
Copy link
Contributor

@marcndo marcndo commented Jun 10, 2025

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

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 marcndo force-pushed the bug/38032-eval_rounding_error branch from 853ad11 to f36e160 Compare June 10, 2025 04:27
@Rocketknight1
Copy link
Member

Rocketknight1 commented Jun 10, 2025

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?

@marcndo
Copy link
Contributor Author

marcndo commented Jun 10, 2025

Sure, I'll proceed with this right away

@marcndo marcndo force-pushed the bug/38032-eval_rounding_error branch 3 times, most recently from 1abfebe to 77acabd Compare June 10, 2025 22:46
@Rocketknight1
Copy link
Member

Looks okay to me now! cc @SunMarc

Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Looks nice. Can you try to check if tools like wandb can interpret this as expected ?

@marcndo
Copy link
Contributor Author

marcndo commented Jun 12, 2025

Certainly, I will proceed with this.

@marcndo
Copy link
Contributor Author

marcndo commented Jun 13, 2025

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 marcndo force-pushed the bug/38032-eval_rounding_error branch from fa137ff to b8f7430 Compare June 13, 2025 03:04
@SunMarc
Copy link
Member

SunMarc commented Jun 13, 2025

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

@marcndo
Copy link
Contributor Author

marcndo commented Jun 13, 2025

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 marcndo force-pushed the bug/38032-eval_rounding_error branch from b8f7430 to 94e2207 Compare June 13, 2025 19:38
@SunMarc
Copy link
Member

SunMarc commented Jun 16, 2025

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

@marcndo
Copy link
Contributor Author

marcndo commented Jun 18, 2025

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

@marcndo
Copy link
Contributor Author

marcndo commented Jun 18, 2025

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

@marcndo
Copy link
Contributor Author

marcndo commented Jun 23, 2025

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!

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.

3 participants