Skip to content
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.

Sign up for GitHub

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

Jump to bottom

truncate long version number in progress bar #2594

Merged
merged 9 commits into from
Jul 28, 2020
Merged

truncate long version number in progress bar #2594

merged 9 commits into from
Jul 28, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Jul 13, 2020 •
edited
Loading

What does this PR do?

Fixes #1595 and adds docs how to override defaults.

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • 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? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@awaelchli awaelchli added feature Is an improvement or enhancement docs Documentation related labels Jul 13, 2020
@mergify mergify bot requested a review from a team July 13, 2020 05:32
@awaelchli awaelchli mentioned this pull request Jul 13, 2020
Closed
@awaelchli awaelchli marked this pull request as ready for review July 13, 2020 05:39
@codecov
Copy link

codecov bot commented Jul 13, 2020 •
edited
Loading

Codecov Report

Merging #2594 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2594   +/-   ##
======================================
  Coverage      90%     91%           
======================================
  Files          82      82           
  Lines        6813    6786   -27     
======================================
- Hits         6164    6153   -11     
+ Misses        649     633   -16

Borda
Borda approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

seems like GH action was not triggered, mind push any commit...

@mergify mergify bot requested a review from a team July 13, 2020 06:29
Borda
Borda reviewed Jul 13, 2020
View reviewed changes
pytorch_lightning/core/lightning.py Outdated
@@ -1588,7 +1603,7 @@ def get_progress_bar_dict(self) -> Dict[str, Union[int, str]]:
tqdm_dict['split_idx'] = self.trainer.split_idx

if self.trainer.logger is not None and self.trainer.logger.version is not None:
tqdm_dict['v_num'] = self.trainer.logger.version
tqdm_dict['v_num'] = str(self.trainer.logger.version)[:3]
Copy link
Member

Choose a reason for hiding this comment

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

btw, why just three?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

github doesn't show it, but I pushed more commits to the branch.
github has some issues at the moment it seems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, why just three?

What would be a better number in your optinion? 5?

Copy link
Member

Choose a reason for hiding this comment

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

I am just trying to understand why not all string...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because some loggers like MLFlow use a very long string as a version and william suggested here to only show the last few bits in the progress bar.

Copy link
Member

Choose a reason for hiding this comment

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

ok, but you show the first three here... :] so str(self.trainer.logger.version)[-3:]
so what about 7 as git does? :]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but you show the first three here... :] so str(self.trainer.logger.version)[-3:]

yes I have that see latest commit.
Let me ask on the original issue what people say about length

@mergify mergify bot requested a review from a team July 13, 2020 06:30
@awaelchli awaelchli marked this pull request as draft July 13, 2020 06:44
@Borda Borda self-requested a review July 13, 2020 06:55
@awaelchli awaelchli force-pushed the feature/truncate-vnum branch from ac1e6b7 to d2021c1 Compare July 13, 2020 10:26
@awaelchli awaelchli marked this pull request as ready for review July 13, 2020 10:29
@Borda Borda changed the title truncate long version number in progress bar truncate long version number in progress bar [wip] Jul 15, 2020
@Borda Borda changed the title truncate long version number in progress bar [wip] truncate long version number in progress bar Jul 15, 2020
@mergify mergify bot requested a review from a team July 15, 2020 12:20
@mergify mergify bot requested a review from a team July 22, 2020 14:08
@Borda Borda force-pushed the feature/truncate-vnum branch from a537ba0 to 475cb46 Compare July 28, 2020 08:06
@williamFalcon williamFalcon merged commit db9f11d into master Jul 28, 2020
@Borda Borda deleted the feature/truncate-vnum branch July 28, 2020 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamFalcon williamFalcon williamFalcon left review comments

@Borda Borda Borda approved these changes

Assignees
No one assigned
Labels
docs Documentation related feature Is an improvement or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to remove v_num from the progress bar ?
3 participants
@awaelchli @williamFalcon @Borda