Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@yanbing-j
Copy link
Contributor

@yanbing-j yanbing-j commented Nov 15, 2024

This PR is to remove tokens per sec in aggregate_metrics when jit_compile, to make the average of tokens per sec more precise. The tokens per sec is too low when jit_compile, which should not be included in aggregate_metrics.

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 15, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchchat/1378

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit fdc416d with merge base 5da240a (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 15, 2024
@yanbing-j
Copy link
Contributor Author

Hi @Jack-Khuu, could you please take a look? Thanks!

@Jack-Khuu
Copy link
Contributor

Thanks for adding. Looks like the compile numbers got added back in (outside of the if) with 77774d2 due to Nan's

I rebased your PR since d7b681a added a Nan check that might actually make the old Nan problem a non-issue

@Jack-Khuu
Copy link
Contributor

Can you add a print near around line ~1210 that tells the user that the averages were calculated with the compile sample excluded?

@yanbing-j
Copy link
Contributor Author

yanbing-j commented Nov 18, 2024

the averages were calculated with the compile sample excluded

Updated. Please take a look again. @Jack-Khuu

@Jack-Khuu
Copy link
Contributor

Jack-Khuu commented Nov 18, 2024

Thanks for the comment

Just one last nit: Let's only print the "excluding compile in calculations" message if the user is actually compiling, to not confuse users

Once that's ready give me a ping and I'll merge this in

@yanbing-j
Copy link
Contributor Author

@Jack-Khuu Thanks for the merging!

@Jack-Khuu Jack-Khuu merged commit a6a6e61 into pytorch:main Nov 19, 2024
52 checks passed
vmpuri pushed a commit that referenced this pull request Feb 4, 2025
* Remove tokens per sec in aggregate_metrics when jit_compile

* Add warning to user

* Update

---------

Co-authored-by: Jack-Khuu <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants