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

Forward tqdm constructor arguments to Progress component #4475

Merged
merged 6 commits into from Jun 26, 2023

Conversation

itrushkin
Copy link
Contributor

Description

Progress component with tqdm tracking enabled could not be rendered when no iterable is specified in tqdm initialization.

This PR forwards the total and unit keyword arguments that are used by Progress.tqdm method.

Example code

import gradio as gr
from tqdm import tqdm
import time

def test(s, _=gr.Progress(track_tqdm=True)):
    a = ''
    with tqdm(total=len(s)) as progress_bar:
        for c in s:
            a = c + a
            progress_bar.update()
            time.sleep(1)
    return a

demo = gr.Interface(test, 'text', 'text')
demo.queue().launch()

Render result

Version Image
main branch image
This PR image

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Signed-off-by: Ilya Trushkin <ilya.trushkin@intel.com>
Signed-off-by: Ilya Trushkin <ilya.trushkin@intel.com>
@gradio-pr-bot
Copy link
Contributor

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4475-all-demos

@@ -546,15 +546,15 @@ def create_tracker(root_blocks, event_id, fn, track_tqdm):
if not hasattr(root_blocks, "_progress_tracker_per_thread"):
root_blocks._progress_tracker_per_thread = {}

def init_tqdm(self, iterable=None, desc=None, *args, **kwargs):
def init_tqdm(self, iterable=None, desc=None, total=None, unit='steps', *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the default value for unit in tqdm.tqdm is it:

Suggested change
def init_tqdm(self, iterable=None, desc=None, total=None, unit='steps', *args, **kwargs):
def init_tqdm(self, iterable=None, desc=None, total=None, unit='it', *args, **kwargs):

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To keep the same behavior, I used the default gradio value for unit used in:

unit: str = "steps",

Should I use tqdm value instead?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm good point. @aliabid94 can you review this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

steps is better, this is what will be displayed in the frontend

@abidlabs
Copy link
Member

Thanks @itrushkin for the nice PR. LGTM and can confirm it solves the original issue. Please see my comment above and in addition, two suggestions:

  • Can you please add a test to test/test_blocks to test this change?
  • Can you please run the formatter scripts (scripts/format_backend.sh) to format and lint the changes?

Should be good to merge then

@shell11708
Copy link

Thanks a lot. To track the progress of hyperparameter tuning with optuna, I applied this PR and added tqdm.set_description wrapper in create_tracker function. Is there any possibility that set_description or set_postfix wrapper will also be added in gr.Progress?

@abidlabs
Copy link
Member

Hi @itrushkin would you be interested in making the suggested changes?

@abidlabs
Copy link
Member

Let us know once you have the test in place @itrushkin

@abidlabs
Copy link
Member

I'll go ahead and make the changes above so that we can merge this in

@gradio-pr-bot
Copy link
Contributor

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4475-all-demos

@abidlabs
Copy link
Member

Merging, thanks again for the contribution @itrushkin

@abidlabs abidlabs merged commit b5121a9 into gradio-app:main Jun 26, 2023
11 checks passed
dawoodkhan82 pushed a commit that referenced this pull request Jun 27, 2023
* Forward tqdm constructor arguments to Progress component

Signed-off-by: Ilya Trushkin <ilya.trushkin@intel.com>

* lint

* reorder args

* added tests

---------

Signed-off-by: Ilya Trushkin <ilya.trushkin@intel.com>
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
dawoodkhan82 added a commit that referenced this pull request Jun 28, 2023
* playwright tests for chatbot

* more tests

* changelog

* Update CHANGELOG.md

* fix upload file delay (#4661)

* fix

* changes

* changes

---------

Co-authored-by: Abubakar Abid <abubakar@huggingface.co>

* Forward `tqdm` constructor arguments to `Progress` component (#4475)

* Forward tqdm constructor arguments to Progress component

Signed-off-by: Ilya Trushkin <ilya.trushkin@intel.com>

* lint

* reorder args

* added tests

---------

Signed-off-by: Ilya Trushkin <ilya.trushkin@intel.com>
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>

* Remove cleared_value (#4685)

* Remove cleared_value

* CHANGELOG

* Add requirements.txt to dialogpt demo (#4686)

* Add requirements.txt

* Update demo notebook

* Add torch

* remove streaming demo + more tests

* Fix blocks_kitchen_sink and streaming_stt demos (#4699)

* Add code

* Add json file

* Remove streaming_stt demo

* Undo generate_notebooks

* Add blocks_kitchen_sink

* fix tests

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* Update js/app/test/chatbot_multimodal.spec.ts

Co-authored-by: pngwn <hello@pngwn.io>

* update notebook

* remove debug

* remove debug

---------

Signed-off-by: Ilya Trushkin <ilya.trushkin@intel.com>
Co-authored-by: aliabid94 <aabid94@gmail.com>
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
Co-authored-by: Ilya Trushkin <ilya.trushkin@intel.com>
Co-authored-by: Freddy Boulton <alfonsoboulton@gmail.com>
Co-authored-by: pngwn <hello@pngwn.io>
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.

None yet

5 participants