Skip to content

[wip] ProgressBar with callable remove_when_done #1025

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

Closed
wants to merge 3 commits into from
Closed

[wip] ProgressBar with callable remove_when_done #1025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a1983a
Initial attempt for a threaded-callable remove_when_done
Dec 10, 2019
643ff1e
Create background tasks with correct event loop
Dec 12, 2019
2c9c57e
typing, remove loop argument, added example
Dec 20, 2019
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion examples/progress-bar/a-lot-of-parallel-tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,28 @@ def main():
bottom_toolbar=HTML("<b>[Control-L]</b> clear <b>[Control-C]</b> abort"),
) as pb:

lock = threading.Lock()
completed = []

# Remove a completed counter after 5 seconds but keep the last 5.
Copy link
Member

Choose a reason for hiding this comment

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

This is nice. That's a great use case.

I'm still not convinced though that a thread is the right approach here. We will get many threads that never finish and can't be joined anywhere. I'm thinking about whether we could reuse the prompt_toolkit filters in this case too, and do something like:

@Condition def ten_seconds_passed() -> bool: counter = get_counter() # context local return time.time() - counter.stop_time > 10 @Condition def at_least_five_bars() -> bool: pb = get_progress_bar() # context local return len(pb.counters) > 10 for i in pb(range(total), label=label, remove_when_done=ten_seconds_passed & at_least_five_bars):

If this condition evaluates during every render, we can remove the bar when this turns False.
def remove_when_done(counter):
counter.label = "Completed"
with lock:
completed.append(counter)
time.sleep(5)
counter.progress_bar.invalidate()
while True:
if len(completed) < 5:
pass
else:
if completed[0] is counter:
with lock:
completed.pop(0)
return True
time.sleep(1)

def run_task(label, total, sleep_time):
for i in pb(range(total), label=label):
for i in pb(range(total), label=label, remove_when_done=remove_when_done):
time.sleep(sleep_time)

threads = []
Expand Down
4 changes: 1 addition & 3 deletions prompt_toolkit/application/application.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@
import prompt_toolkit.eventloop.dummy_contextvars as contextvars # type: ignore


__all__ = [
"Application",
]
__all__ = ["Application"]


E = KeyPressEvent
Expand Down
43 changes: 33 additions & 10 deletions prompt_toolkit/shortcuts/progress_bar/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import traceback
from asyncio import get_event_loop, new_event_loop, set_event_loop
from typing import (
Callable,
Generic,
Iterable,
List,
Expand All @@ -23,11 +24,13 @@
Sized,
TextIO,
TypeVar,
Union,
cast,
)

from prompt_toolkit.application import Application
from prompt_toolkit.application import Application, get_app
from prompt_toolkit.application.current import get_app_session
from prompt_toolkit.eventloop import run_in_executor_with_context
from prompt_toolkit.filters import Condition, is_done, renderer_height_is_known
from prompt_toolkit.formatted_text import (
AnyFormattedText,
Expand Down Expand Up @@ -56,12 +59,10 @@
try:
import contextvars
except ImportError:
from prompt_toolkit.eventloop import dummy_contextvars as contextvars # type: ignore
import prompt_toolkit.eventloop.dummy_contextvars as contextvars # type: ignore


__all__ = [
"ProgressBar",
]
__all__ = ["ProgressBar"]

E = KeyPressEvent

Expand Down Expand Up @@ -250,15 +251,19 @@ def __call__(
self,
data: Optional[Iterable[_T]] = None,
label: AnyFormattedText = "",
remove_when_done: bool = False,
remove_when_done: Union[
Callable[["ProgressBarCounter[_T]"], bool], bool
] = False,
total: Optional[int] = None,
) -> "ProgressBarCounter[_T]":
"""
Start a new counter.

:param label: Title text or description for this progress. (This can be
formatted text as well).
:param remove_when_done: When `True`, hide this progress bar.
:param remove_when_done: When `True`, hide this progress bar. Can be a
callable accepting a ProgressBarCounter instance. The callable is
run in the background allowing for delayed removals.
:param total: Specify the maximum value if it can't be calculated by
calling ``len``.
"""
Expand Down Expand Up @@ -319,7 +324,9 @@ def __init__(
progress_bar: ProgressBar,
data: Optional[Iterable[_CounterItem]] = None,
label: AnyFormattedText = "",
remove_when_done: bool = False,
remove_when_done: Union[
Callable[["ProgressBarCounter[_CounterItem]"], bool], bool
] = False,
total: Optional[int] = None,
) -> None:

Expand Down Expand Up @@ -359,6 +366,13 @@ def item_completed(self) -> None:
self.items_completed += 1
self.progress_bar.invalidate()

async def _remove_when_done_async(self) -> None:
def run_remove_when_done_thread() -> bool:
return self.remove_when_done(self)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't correspond with the type signature, I think it's fine to pass self, but the Callable in __init__ should match.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm I think I got this figured out

if await run_in_executor_with_context(run_remove_when_done_thread):
Copy link
Member

Choose a reason for hiding this comment

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

I'm still thinking about whether this is the right thing to do.

By default, I'd like as much as possible for prompt_toolkit to have asynchronous APIs, and possibly create an synchronous abstraction on top of that. If we expect a blocking synchronous function here, then we exclude people that would like to include it in an async app.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I borrowed almost all of this code from how validation and ThreadedValidators are implemented. I do see that the current implementation appears to have some issue with cleaning up tasks quickly upon completion (you'll see this if you run the a-lot-of-parallel-tasks.py example that I updated).

Would it be better to allow remove_when_done to be either synchronous or asynchronous and then it's up to the user to design any expensive async functions correctly (with asyncio.sleep, etc.)?
self.progress_bar.counters.remove(self)

@property
def done(self) -> bool:
return self._done
Expand All @@ -370,8 +384,17 @@ def done(self, value: bool) -> None:
# If done then store the stop_time, otherwise clear.
self.stop_time = datetime.datetime.now() if value else None

if value and self.remove_when_done:
self.progress_bar.counters.remove(self)
if value:
if callable(self.remove_when_done):
# Register a background task to run the remove_when_done callable.
self.progress_bar._app_loop.call_soon_threadsafe(
lambda: self.progress_bar.app.create_background_task(
self._remove_when_done_async()
)
)
elif self.remove_when_done:
# Non-callable that is True, remove bar.
self.progress_bar.counters.remove(self)

@property
def percentage(self) -> float:
Expand Down