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

Tqdm Indicator #2079

Merged
merged 19 commits into from Jun 30, 2021
Merged

Tqdm Indicator #2079

merged 19 commits into from Jun 30, 2021

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Mar 13, 2021

One very powerful tool for progress reporting is tqdm. It gives very nice reporting for loops and also for a lot of numpy/ Pandas operations.

I would like to have the same capabilities inside my Panel apps whether its on the server or in a notebook. But especially in combination with the new --auto flag that makes the panel server really suited for exploratory work also it would be really nice to display tqdm in the app.

So this is a PR to contribute the Tqdm Progress Indicator

tqdm-progress2

Must Do:

[x] Notebook Reference Guide
[x] Consider renaming TQDMProgress to TQDM.
[x] Rename to Tqdm.
[x] Remove initial not active indication.
[x] Move test to test file.
[x] Fix problems that ipywidgets are required but not used in notebook

Nice to Have

[] Set value, max and text parameters to constant
- When I try I get TypeError: Constant parameter 'max' cannot be modified even though I use param.edit_constant. DON'T KNOW HOW TO FIX THIS.
[] Fix layout/ margin issues for "row" layout in notebook (see pictures in discussion below). DON'T KNOW HOW TO FIX THIS.
[x] Consider whether adding __call__ is good or bad for the user.
- With call it's easier to use but it also might confuse some users as Tqdm.tqdm now contains the pandas method. (C.f. notebook). I WILL KEEP IT. I THINK ITS OK.
[x] Consider converting to Bokeh Extension instead of Viewer. WONT DO.
[x] Move to indicators.py file (throws errors if I try).
[x] Inherit from Indicator (throws errors if I try). WON'T DO. Tqdm is a Viewer and not a Widget.
[] Verify that nested tqdm works.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Mar 13, 2021

Alternative API.

Originally I would have liked the api to be, i.e. tqdm instead of tqdm.tqdm in the loop. But I could not figure out how to implement it.

 tqdm = TQDMProgress(width=800, layout="column")
    def run(*events):
        for index in tqdm(range(0,10)):
            time.sleep(0.2)

But I ended up with.

 tqdm = TQDMProgress(width=800, layout="column")
    def run(*events):
        for index in tqdm.tqdm(range(0,10)):
            time.sleep(0.2)

I don't think it will make a difference. Maybe the api I ended up with makes more sense.

@hoxbro
Copy link
Member

hoxbro commented Mar 13, 2021

This looks pretty cool!

To get it to works with tqdm you could add a __call__ to your TQDMProgress class like this:

    def __call__(self, *args, **kwargs):
        return self.tqdm(*args, **kwargs)

@MarcSkovMadsen
Copy link
Collaborator Author

Thanks @hoxbro . It was actually __call__ I tried but could not get working. But powered by the inspiration of your kind words I will try again :-)

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Mar 13, 2021

Got it working @hoxbro . It was easy. Maybe I forgot self in the first place.

I think the simplification is worth it.

tqdm-progress4.mp4

UPDATE: I am now in doubt whether __call__ will help the user or confuse the user. Because for example to add support for pandas the user has to run tqdm.tqdm.pandas().

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Mar 13, 2021

Added notebook documentation.

tqdm-reference.mp4

@casperdcl
Copy link

as a minor point, tqdm is used as a word rather than an acronym (so tqdm or Tqdm but never TQDM)

tqdm means "progress" in Arabic (taqadum, تقدّم) and is an abbreviation for "I love you so much" in Spanish (te quiero demasiado).

😜

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Mar 13, 2021

Ahh. Thanks @casperdcl. I will rename to Tqdm ❤️😀

@MarcSkovMadsen MarcSkovMadsen changed the title TQDM Progress Indicator Tqdm Indicator Mar 13, 2021
@MarcSkovMadsen
Copy link
Collaborator Author

One issue I don't know how to solve in the current implementation is the "row" alignment in the notebook. It looks like

image

but on server it looks like

image

@MarcSkovMadsen
Copy link
Collaborator Author

Hi @philippjfr

The PR is now at a stage where it would be helpful with a first review and some feedback on

  • whether you would like the Tqdm indicator in Panel
  • what "must" be done before it is ready for acceptance.

You can see the list of outstanding things that would be nice to solve at the top. I am stuck there solving those things and would need some hints, guidance or help there.

I don't plan to do more work before I get the feedback :-) Thanks.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Mar 14, 2021

FYI. @philippjfr I have implemented your suggestions. Thanks.

There are two minor, nice to have items (see todo at top) that I don't know how to fix. I don't think they are important.

Let me know if there is more I should do. I don't plan to do more on this. I think its fine.

@philippjfr philippjfr merged commit d98bb0b into master Jun 30, 2021
@philippjfr philippjfr deleted the tqdm-indicator branch June 30, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants