-
Notifications
You must be signed in to change notification settings - Fork 49
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
Progress bar tqdm wrapper, and manual control #105
Conversation
Codecov Report
@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 90.51% 90.78% +0.26%
==========================================
Files 25 26 +1
Lines 2435 2517 +82
==========================================
+ Hits 2204 2285 +81
- Misses 231 232 +1
Continue to review full report at Codecov.
|
this is getting kinda cool! import random
from time import sleep
from magicgui import magicgui
from magicgui.tqdm import tmgrange, tqdm_mgui
@magicgui(call_button=True, layout="vertical")
def long_function(
steps=20, repeats=4, choices="ABCDEFGHIJKLMNOP12345679", char="", delay=0.1
):
# using the `tmgrange` shortcut instead of range()
for r in tmgrange(repeats, label="repeats"):
letters = [random.choice(choices) for _ in range(steps)]
# `tqdm_gui`, like `tqdm`, accepts any iterable
# this progress bar is nested and will be run & reset multiple times
for letter in tqdm_mgui(letters, label="steps"):
long_function.char.value = letter
sleep(delay)
long_function.show(run=True) Untitled.mov |
progress bar can now also be controlled by annotating a parameter as @magicgui(call_button="tick", pbar={"min": 0, "step": 2, "max": 20, "value": 0})
def manual(pbar: ProgressBar, increment: bool = True):
"""Example of manual progress bar control."""
if increment:
# can also use pbar.value, pbar.min, pbar.max, etc...
pbar.increment()
else:
pbar.decrement() |
Wow, this is looking really cool!! I can give a proper review later, but overall the functionality looks great. I'll need to do a little investigation to understand when I'd use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this is looking good i left some comments/ questions, most are for my clarification/ naming ideas that could be ignored - probably the only thing i see as critical is enable/ disable idea for fixing the segfault after clicking run twice.
sleep(delay) | ||
|
||
|
||
long_running.show(run=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that if I click the run button while the thing is running i get a seg fault and quit - we should probably just disable the button while the loop starts and then enable it when done, possibly with a context manager so that it re-enables even with an error during the iteration
(base) czirwc1macos2701:magicgui nsofroniew$ python examples/progress.py
Traceback (most recent call last):
File "/Users/nsofroniew/GitHub/magicgui/magicgui/widgets/_bases/value_widget.py", line 44, in <lambda>
lambda *x: self.changed(value=x[0] if x else None)
File "/Users/nsofroniew/GitHub/magicgui/magicgui/events.py", line 576, in __call__
raise RuntimeError("EventEmitter loop detected!")
RuntimeError: EventEmitter loop detected!
Abort trap: 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I noticed that too. That's a larger issue having to do with long running functions that doesn't have to do with this PR ... will make a new issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that issue might need to get fixed first, but thinking about it more I think i'd like the behavior to toggle run/ pause
which would start and stop progress. could come in later PRs, but noting it now
@@ -0,0 +1,151 @@ | |||
"""A wrapper around the tqdm.tqdm iterator that adds a ProgressBar to a magicgui.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if there is another place this could live that is not at the top level, i.e. a grouping of a larger thing that it might be an example of? I guess though there isn't anything else quite like in magicgui right now as it's not a widget per se (that is ProgressBar
itself, this is the iterator) but something that if found inside magic functions with generate a widget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess though there isn't anything else quite like in magicgui right now as it's not a widget per se
exactly. this is just the iterator magic. the main reason I put it at the top level is that I don't want to actually depend on tqdm
directly (it's available as magicgui[tqdm]
), and I want people to use from magicgui.tqdm import tqdm...
. As you point out, you can still use the manual progressbar API without this (magicgui.widgets.ProgresBar)
magicgui/tqdm.py
Outdated
} | ||
|
||
|
||
class tqdm_mgui(tqdm): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why not just have this class be called tqdm
, to use it you're importing from magicgui import tqdm
already, especially if this is a drop in replacement for tqdm
/ behavior is identical if not inside a magicgui decorated function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's not yet a drop-in replacement :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok now it is! I liked this idea... so now it's just from magicgui.tqdm import tqdm, trange
... and if it's not inside of a @magicgui
it will work as usual (like in IPython)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love it so much!!
magicgui/tqdm.py
Outdated
self._mgui._tqdm_depth -= 1 | ||
|
||
|
||
def tmgrange(*args, **kwargs) -> tqdm_mgui: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is analogous to trange
, might be nice to link to that https://tqdm.github.io/docs/tqdm/#trange somehow. I'm also wondering if the above import were to change to from magicgui import tqdm
would this be from magicgui import trange
, maybe not, that's a little awkward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above about not depending directly on tqdm. I wanted the import error to be raised on accessing magicgui.tqdm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot!
sleep(delay) | ||
|
||
|
||
long_running.show(run=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I noticed that too. That's a larger issue having to do with long running functions that doesn't have to do with this PR ... will make a new issue
@@ -0,0 +1,151 @@ | |||
"""A wrapper around the tqdm.tqdm iterator that adds a ProgressBar to a magicgui.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess though there isn't anything else quite like in magicgui right now as it's not a widget per se
exactly. this is just the iterator magic. the main reason I put it at the top level is that I don't want to actually depend on tqdm
directly (it's available as magicgui[tqdm]
), and I want people to use from magicgui.tqdm import tqdm...
. As you point out, you can still use the manual progressbar API without this (magicgui.widgets.ProgresBar)
magicgui/tqdm.py
Outdated
} | ||
|
||
|
||
class tqdm_mgui(tqdm): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's not yet a drop-in replacement :/
magicgui/tqdm.py
Outdated
self._mgui._tqdm_depth -= 1 | ||
|
||
|
||
def tmgrange(*args, **kwargs) -> tqdm_mgui: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above about not depending directly on tqdm. I wanted the import error to be raised on accessing magicgui.tqdm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I love it - looks great to me. I made an additional comment on the run/ pause
toggle, but sounds like that might have to come in a new PR. I think that's fine, but depending on how much work it is might try and get it in before the next release too. I find it very tempting to click on the run button again and imagine others will too :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tlambert03 pretty happy with this. I'm not 100% about "overwriting" the tqdm name, but I see the argument in favour, so let's give it a go.
Overall, I was kind of envisioning a different, more magical API using generators. Half baked idea:
@magicgui(
progress=[argname], # where to get the total length
)
def generator(argname : List):
for elem in list:
process(elem)
yield
return
There's every chance this current API is better though; For one, my suggestion can't do nested iterations, or resetting, or any of that. But, maybe it could be extended (multiple args, yielded values). At any rate, these are all things that can be explored in the future.
(as discussed on zulip), I like this idea too, and agree that it isn't mutually exclusive. I like that the current pattern lets someone simply wrap the iterator itself (i.e. it doesn't require you to specify the parameters of the iterable in two places), but I also like thinking about what we could do with generators. so I'll merge this, and keep thinking about that as a complementary pattern |
this PR adds a basic
tqdm_mgui
iterator that updates a progress bar (from #104), with the internal state of the tqdm iterator. allowing iterator-based progress bars ( a step towards #25)tqdm.mov