Skip to content

[WIP] fix progress bar lacks a wrap #1496

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[WIP] fix progress bar lacks a wrap #1496

wants to merge 5 commits into from

Conversation

teamwong111
Copy link
Contributor

@teamwong111 teamwong111 commented Nov 16, 2021 •
edited
Loading

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

Related issue: #988.
Fix the progress bar lacks a wrap.

Modification

Add wrap in the corresponding function.

BC-breaking (Optional)

Honestly, I am not quite sure about whether this PR introduces BC-breaking because some downstream repos impl their own single_gpu_test and multi_gpu_test but maybe other downstream repos use these func in mmcv. However, we should ask them to use the progressbar according to the new Impl of progressbar.

Use cases (Optional)

Modify corresponding config files

workflow = [('train', 1)] --> workflow = [('train', 1), ('val', 1)]
EpochBasedRunner --> IterBasedRunner

Run

python tools/train.py configs/resnet/resnet18_b16x8_cifar10.py

Before

[>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>] 10000/10000, 2684.9 task/s, elapsed: 4s, ETA:     0s2021-11-17 10:49:22,284 - mmcls - INFO - Iter(val) [625]       accuracy_top-1: 10.0000, accuracy_top-5: 50.8100

After

[>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>] 10000/10000, 2658.6 task/s, elapsed: 4s, ETA:     0s
2021-11-17 10:48:16,768 - mmcls - INFO - Iter(val) [625]        accuracy_top-1: 12.9500, accuracy_top-5: 50.0000

Update: test for multi_gpu_test()

[>>>>>>>>>>>>>>>>>>>>>>>>] 10000/10000, 42.8 task/s, elapsed: 234s, ETA:     0s  
2021-12-01 13:05:47,997 - mmcls - INFO - Iter(val) [625] accuracy_top-1: 10.0000, accuracy_top-5: 50.0000
2021-12-01 13:05:52,824 - mmcls - INFO - Saving checkpoint at 2 iterations

Checklist

Before PR:

  • I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
  • Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
  • New functionalities are covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • The documentation has been modified accordingly, including docstring or example tutorials.

After PR:

  • If the modification has potential influence on downstream or other related projects, this PR should be tested with some of those projects, like MMDet or MMCls.
  • CLA has been signed and all committers have signed the CLA in this PR.

ZwwWayne
ZwwWayne reviewed Nov 19, 2021
View reviewed changes
@ZwwWayne
Copy link
Collaborator

Does this PR introduces BC-breaking? For example, there are many code in downstream repos using the old style like

if rank == 0:
        prog_bar = mmcv.ProgressBar(len(dataset))
for I in xxx:
    prog_bar.update()

What will happen if we do not modify our usage of progressbar?

@teamwong111
Copy link
Contributor Author

teamwong111 commented Nov 19, 2021 •
edited
Loading

Does this PR introduces BC-breaking? For example, there are many code in downstream repos using the old style like

if rank == 0:
        prog_bar = mmcv.ProgressBar(len(dataset))
for I in xxx:
    prog_bar.update()

What will happen if we do not modify our usage of progressbar?

Honestly, I am not quite sure about whether this PR introduces BC-breaking because some downstream repos impl their own single_gpu_test and multi_gpu_test but maybe other downstream repos use these func in mmcv. However, we should ask them to use the progressbar according to the new Impl of progressbar.

@zhouzaida
Copy link
Collaborator

Does this PR introduces BC-breaking? For example, there are many code in downstream repos using the old style like

if rank == 0:
        prog_bar = mmcv.ProgressBar(len(dataset))
for I in xxx:
    prog_bar.update()

What will happen if we do not modify our usage of progressbar?

I think it will not introduce BC-breaking. In fact, the PR just adds a \n character aotumatically if we use the with clause to manage the context. If we do not use the with clause, everything is the same as before.

@teamwong111 teamwong111 linked an issue Dec 10, 2021 that may be closed by this pull request
Progress bar does not produce a new line after it finishes #988
Open
@ZwwWayne
Copy link
Collaborator

Ready to merge, we can choose either adding documentations in this PR or update it in another PR.

@zhouzaida zhouzaida changed the title [Fix] fix progress bar lacks a wrap [WIP] fix progress bar lacks a wrap Dec 22, 2021
@zhouzaida zhouzaida added the WIP label Dec 22, 2021
@teamwong111
Copy link
Contributor Author

teamwong111 commented Dec 23, 2021 •
edited
Loading

Ready to merge, we can choose either adding documentations in this PR or update it in another PR.

What kind of docs, about what we modified and how downstream to modify? And where should we put the docs, compatibility.md?

@ZwwWayne
Copy link
Collaborator

Sorry for the vague comments. Shall we update this documentation https://github.com/open-mmlab/mmcv/blob/master/docs/en/understand_mmcv/utils.md#progressbar to tell users about the new and old usages of progressbar?

@teamwong111
Copy link
Contributor Author

Sorry for the vague comments. Shall we update this documentation https://github.com/open-mmlab/mmcv/blob/master/docs/en/understand_mmcv/utils.md#progressbar to tell users about the new and old usages of progressbar?

In the above docs we only provide the usages of track_progress, track_parallel_progress and track_iter_progress, their usages and the previous remain the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouzaida zhouzaida zhouzaida approved these changes

@ZwwWayne ZwwWayne ZwwWayne approved these changes

Assignees
No one assigned
Labels
WIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Progress bar does not produce a new line after it finishes
3 participants
@teamwong111 @ZwwWayne @zhouzaida