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.

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

Show progress #641

Merged
merged 8 commits into from
May 17, 2022
Merged

Show progress #641

merged 8 commits into from
May 17, 2022

Conversation

michalgregor
Copy link
Contributor

Hi, I added support for disabling the tqdm progress bar in the base trainer by replacing it with a dummy class that just keeps track of the total and n stats, but doesn't display progress.

I wonder whether you would like to merge this.

  • A dummy_tqdm class added to utils: it replicates the interface used
    by trainers, but does not show the progress bar;
  • Added a show_progress argument to the base trainer: when
    show_progress == True, dummy_tqdm is used in place of tqdm;

Btw. I wonder whether requiring make format and then not actually doing it on every commit is a good idea – I think it is going to make the changes I made more difficult to find – but at least I did that step in a separate commit, so that should help.

  • I have marked all applicable categories:
    • exception-raising fix
    • algorithm implementation fix
    • documentation modification
    • new feature
  • I have reformatted the code using make format (required)
  • I have checked the code using make commit-checks (required)
  • If applicable, I have mentioned the relevant/related issue(s)
  • If applicable, I have listed every items in this Pull Request below

* A dummy_tqdm class added to utils: it replicates the interface used
  by  trainers, but does not show the progress bar;
* Added a show_progress argument to the base trainer: when
  show_progress == True,  dummy_tqdm is used in place of tqdm;
@Trinkle23897
Copy link
Collaborator

Trinkle23897 commented May 17, 2022

Did you use the old version isort?
I use isort 5.9.3, yapf 0.31.0, python3.8, ubuntu 2004

@Trinkle23897
Copy link
Collaborator

Why not use disable? https://stackoverflow.com/a/61109243/8773993

@michalgregor
Copy link
Contributor Author

Disable does not seem to work – not the way we are using the interface here. With disabled, n does not get incremented when doing update:

if self.disable:
            return

        if n < 0:
            self.last_print_n += n  # for auto-refresh logic to work
        self.n += n

https://github.com/tqdm/tqdm/blob/master/tqdm/std.py

@michalgregor
Copy link
Contributor Author

Did you use the old version isort? I use isort 5.9.3, yapf 0.31.0, python3.8, ubuntu 2004

I think you are right! I assumed it was among the packages updated automatically by the script. I will make sure my versions are as close to yours as possible. What should I do then? Do I need to rerun make format and make commit-checks with them?

@Trinkle23897
Copy link
Collaborator

Trinkle23897 commented May 17, 2022

pip3 install isort yapf --upgrade

See the version here: https://github.com/thu-ml/tianshou/runs/6470112919?check_suite_focus=true (Install dependencies)

Collecting yapf
  Downloading yapf-0.32.0-py2.py3-none-any.whl (190 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 190.2/190.2 kB 37.8 MB/s eta 0:00:00
Collecting isort
  Downloading isort-5.10.1-py3-none-any.whl (103 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 103.4/103.4 kB 22.3 MB/s eta 0:00:00

@Trinkle23897
Copy link
Collaborator

Trinkle23897 commented May 17, 2022

Disable does not seem to work – not the way we are using the interface here. With disabled, n does not get incremented when doing update:

         # perform n step_per_epoch
         with progress(
-            total=self.step_per_epoch, desc=f"Epoch #{self.epoch}", **tqdm_config
+            total=self.step_per_epoch, desc=f"Epoch #{self.epoch}", **tqdm_config, disable=True
         ) as t:

I manually tested with disable=True and dummy_tqdm. I didn't find any difference between these two approaches.

@michalgregor
Copy link
Contributor Author

Are you sure you are in fact running the disable=True on the tqdm version and not the dummy version? Because for me, with disable=True, episodes never terminate – which is what I would expect with t.n not getting updated.

@Trinkle23897
Copy link
Collaborator

Yes you're right. I'll take it.

@codecov-commenter
Copy link

Codecov Report

Merging #641 (028b8c9) into master (53e6b04) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
+ Coverage   93.59%   93.62%   +0.02%     
==========================================
  Files          71       71              
  Lines        4733     4750      +17     
==========================================
+ Hits         4430     4447      +17     
  Misses        303      303              
Flag Coverage Δ
unittests 93.62% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tianshou/trainer/offline.py 100.00% <ø> (ø)
tianshou/trainer/offpolicy.py 100.00% <ø> (ø)
tianshou/trainer/onpolicy.py 100.00% <ø> (ø)
tianshou/trainer/base.py 96.85% <100.00%> (+0.06%) ⬆️
tianshou/utils/__init__.py 100.00% <100.00%> (ø)
tianshou/utils/progress_bar.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Trinkle23897 Trinkle23897 merged commit c87b9f4 into thu-ml:master May 17, 2022
BFAnas pushed a commit to BFAnas/tianshou that referenced this pull request May 5, 2024
- A DummyTqdm class added to utils: it replicates the interface used by trainers, but does not show the progress bar;
- Added a show_progress argument to the base trainer: when show_progress == True, dummy_tqdm is used in place of tqdm.
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