Skip to content

Conversation

@AsherJingkongChen
Copy link

@AsherJingkongChen AsherJingkongChen commented Jan 3, 2026

What does this PR do?

RichModelSummary incorrectly passed model_size (already in MB) to get_human_readable_count(), which converts numbers to K/M/G format. This resulted in misleading output like "5.5 K" instead of the actual size "5,500.000".

Now uses get_formatted_model_size() for consistency with ModelSummary.

Before submitting
  • Was this discussed/agreed via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

📚 Documentation preview 📚: https://pytorch-lightning--21467.org.readthedocs.build/en/21467/

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Jan 3, 2026
@codecov
Copy link

codecov bot commented Jan 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79%. Comparing base (9df1910) to head (bd1b8f9).
✅ All tests successful. No failed tests found.

❗ There is a different number of reports uploaded between BASE (9df1910) and HEAD (bd1b8f9). Click for more details.

HEAD has 429 uploads less than BASE
Flag BASE (9df1910) HEAD (bd1b8f9)
cpu 132 33
python 12 3
lightning_fabric 36 0
pytest 66 0
python3.12 36 9
python3.10 12 3
lightning 60 15
python3.12.7 36 9
python3.11 24 6
python3.13 12 3
pytorch2.2.2 6 3
pytest-full 66 33
pytorch2.8 12 6
pytorch_lightning 36 18
pytorch2.4.1 6 3
pytorch2.1 12 6
pytorch2.7 6 3
pytorch2.6 6 3
pytorch2.3 6 3
pytorch2.9 6 3
pytorch2.5.1 6 3
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #21467     +/-   ##
=========================================
- Coverage      87%      79%     -8%     
=========================================
  Files         270      267      -3     
  Lines       24059    24005     -54     
=========================================
- Hits        20855    18953   -1902     
- Misses       3204     5052   +1848     

Copy link
Collaborator

@bhimrazy bhimrazy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Can you add a test ?

@AsherJingkongChen AsherJingkongChen force-pushed the fix/rich-model-summary-size-display branch from 4a9d305 to 3c49304 Compare January 12, 2026 10:24
@deependujha deependujha added the ready to be merged PRs ready to be merged label Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pl Generic label for PyTorch Lightning package ready to be merged PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants