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

Calculate ProgressBar ETA from most recent updates rather than for whole period #4120

Closed
wants to merge 44 commits into from
Closed
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
4162071
Add a reset method to the ETAStatus widget
davep Feb 5, 2024
d655d82
Provide an interface for requesting an ETA reset
davep Feb 5, 2024
eaf561a
Switch progress ETA calculation to using a set of recent samples
davep Feb 5, 2024
652804d
Fix how we check if we're showing the ETA
davep Feb 5, 2024
bd1ec0d
Actually stop the ETA refresh timer
davep Feb 5, 2024
91e80ea
Don't show the progress ETA in the tooltip snapshop test
davep Feb 5, 2024
f593480
Adapt some progress examples to the new ETA approach
davep Feb 5, 2024
f783284
Revert "Adapt some progress examples to the new ETA approach"
davep Feb 5, 2024
b030501
Add a class for estimating a time to completion
davep Feb 6, 2024
5347989
Have ETC speeds fall back to elapsed
davep Feb 6, 2024
d21b0b4
Be very clear for the reader how distance remaining is calculated
davep Feb 6, 2024
36fb880
Rename distance_covered to distance_covered_in_window
davep Feb 6, 2024
175c779
Tweak the window size of the ETC calculation
davep Feb 6, 2024
f82bddf
Fix trying to get a sample that doesn't yet exist
davep Feb 6, 2024
a64239e
Plug the ETC code into ProgressBar
davep Feb 6, 2024
d3875ad
Update the doc screenshot apps for the new ETAStatus
davep Feb 7, 2024
759fe7b
Revert to looking at the full range of time (for now anyway)
davep Feb 7, 2024
706862e
Add some initial unit tests for TimeToCompletion
davep Feb 7, 2024
1388889
Rename window_size to sample_window_size
davep Feb 7, 2024
12a0b03
Remove the "as of now" tests; they make zero sense in testing
davep Feb 7, 2024
91376a1
Move to a ETC calculation that also takes a window of time into account
davep Feb 7, 2024
4cdb82d
Add Rich repr support to TimeToCompletion
davep Feb 7, 2024
a1d56b8
Swap to using a window of time and max samples
davep Feb 7, 2024
f266bd3
Add some missing return sections to docstrings
davep Feb 7, 2024
7a0a1a5
Fall back to all available samples if they're too old
davep Feb 7, 2024
57d0cae
Rework samples window to be time-first, samples-constrained
davep Feb 8, 2024
a5f4a61
Update ProgressBar snapshot tests
davep Feb 8, 2024
401db02
Make a single sample the whole window to help calculation
davep Feb 8, 2024
aed2bb5
Handle situations where the sample window gets too narrow
davep Feb 9, 2024
8532e57
Clear out the last-dropped sample when clearing samples
davep Feb 9, 2024
818ade5
Add a test for the too-narrow window fallback estimation
davep Feb 9, 2024
188f0d9
Fix a typo
davep Feb 9, 2024
ba5680f
Tidy up some docstrings
davep Feb 9, 2024
a6717aa
Expand the docstring a wee bit
davep Feb 12, 2024
da1f6ab
Apply the no TLA rule
davep Feb 12, 2024
40cf23a
Reduce property lookups in the recording method
davep Feb 12, 2024
e8e5a25
Add a Raises section to the record docstring
davep Feb 12, 2024
40012f2
Merge branch 'main' into progress-progress
davep Feb 12, 2024
8e94605
Docstring tidying
davep Feb 12, 2024
6284fd0
Apply the no-TLA rule to the tests for ETC
davep Feb 12, 2024
4db035c
Make it explicit that the backward value test is about the value
davep Feb 12, 2024
856d8c0
Don't allow time to go backwards when recording a sample
davep Feb 12, 2024
ceda0f9
Swap the Sample class to being a NamedTuple
davep Feb 12, 2024
5eedaf4
Merge branch 'main' into progress-progress
davep Feb 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion docs/examples/widgets/progress_bar_isolated_.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
from textual._time_to_completion import TimeToCompletion
from textual.app import App, ComposeResult
from textual.containers import Center, Middle
from textual.timer import Timer
from textual.widgets import Footer, ProgressBar


class MockETC(TimeToCompletion):

@property
def estimated_time_to_complete_as_of_now(self) -> float:
return 7.0


class IndeterminateProgressBar(App[None]):
BINDINGS = [("s", "start", "Start")]

Expand Down Expand Up @@ -35,7 +43,7 @@ def key_f(self) -> None:

def key_t(self) -> None:
# Freeze time to show always the same ETA.
self.query_one(ProgressBar).query_one("#eta")._get_elapsed_time = lambda: 3.9
self.query_one(ProgressBar).query_one("#eta")._samples = MockETC(100)
self.query_one(ProgressBar).update(total=100, progress=39)

def key_u(self) -> None:
Expand Down
10 changes: 9 additions & 1 deletion docs/examples/widgets/progress_bar_styled_.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
from textual._time_to_completion import TimeToCompletion
from textual.app import App, ComposeResult
from textual.containers import Center, Middle
from textual.timer import Timer
from textual.widgets import Footer, ProgressBar


class MockETC(TimeToCompletion):

@property
def estimated_time_to_complete_as_of_now(self) -> float:
return 7.0


class StyledProgressBar(App[None]):
BINDINGS = [("s", "start", "Start")]
CSS_PATH = "progress_bar_styled.tcss"
Expand Down Expand Up @@ -36,7 +44,7 @@ def key_f(self) -> None:

def key_t(self) -> None:
# Freeze time to show always the same ETA.
self.query_one(ProgressBar).query_one("#eta")._get_elapsed_time = lambda: 3.9
self.query_one(ProgressBar).query_one("#eta")._samples = MockETC(100)
self.query_one(ProgressBar).update(total=100, progress=39)

def key_u(self) -> None:
Expand Down
301 changes: 301 additions & 0 deletions src/textual/_time_to_completion.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,301 @@
"""Code to help calculate the estimated time to completion of some process."""

from __future__ import annotations

from dataclasses import dataclass
from time import monotonic

from rich.repr import Result
from typing_extensions import Self


@dataclass
class Sample:
"""A sample."""

value: float
"""The value of the sample."""

moment: float
"""The moment when the sample was taken."""


class Samples:
"""A deque-ish-like object that holds samples.

The window for the samples is based on time, with a maximum sample cap
within that window to ensure memory use doesn't run away. Also, for
times when the sample window starts to run dry (drops to just the one
available sample in the window) the [last-dropped
sample][textual._time_to_completion.Samples.last_dropped] is held on to
and made available.
"""

def __init__(self, time_window_size: float, max_samples: int) -> None:
"""Initialise the samples object.

Args:
time_window_size: The window of time to keep samples for.
max_samples: The maximum number of samples to keep.
"""
self._time_window_size = time_window_size
"""The maximum amount of time to keep the samples for."""
self._max_samples = max_samples
"""The maximum number of samples to keep."""
self._samples: list[Sample] = []
"""The samples."""
self._last_dropped: Sample | None = None
"""Keep track of the last-dropped sample."""

def _prune(self) -> Self:
"""Prune the samples.

Returns:
Self.
"""
if samples := self._samples:
# Trim off any "too old" samples.
oldest_time = samples[-1].moment - self._time_window_size
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the pruning really occur from the last sample, or should it occur from the current time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to me that it works off the last-known sample rather than off "now", as what we're making here is a heuristic for figuring out a likely time to completion, and going off "last best" makes more sense to me than going off nothing.

for position, sample in enumerate(samples):
if sample.moment > oldest_time:
# We keep the "last dropped" sample around for any sort
# of fallback position.
self._last_dropped = samples[position - 1]
self._samples = samples[position:]
break
# Ensure that we don't run up too many samples.
self._samples = self._samples[-self._max_samples :]
return self

def append(self, sample: Sample) -> Self:
"""Add a sample to the samples.

Args:
sample: The sample to add.

Returns:
Self.
"""
self._samples.append(sample)
return self._prune()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to call prune on every sample?

Could we defer pruning to when we need the samples? i.e. if there are 1000 samples between refreshes, could we prune just once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could go either way; there could be 1,000 "refreshes" between samples too. It feels to me like, on balance, code that is showing an estimated time is more likely to get new estimates than it is to record new samples.


def clear(self) -> Self:
"""Clear the samples.

Returns:
Self.
"""
self._samples.clear()
self._last_dropped = None
return self

@property
def last_dropped(self) -> Sample | None:
"""The last sample to be dropped out of the time window.

If none have been dropped yet then `None`.
"""
return self._last_dropped

def __getitem__(self, index: int) -> Sample:
return self._samples[index]

def __len__(self) -> int:
return len(self._samples)

def __rich_repr__(self) -> Result:
yield self._samples


class TimeToCompletion:
"""A class for calculating the time to completion of something.

A utility class designed to help calculate the time to completion of a
series of points that happen over time. Values recorded are assumed to
be >= 0.
"""

def __init__(
self,
destination: float,
*,
time_window_size: float = 30,
max_samples: int = 1_000,
) -> None:
"""Initialise the time to completion object.

Args:
destination: The destination value.
time_window_size: The size of the time window to work off.
max_samples: The maximum number of samples to retain.
"""
self._destination = destination
"""The destination value."""
self._samples = Samples(time_window_size, max_samples)
"""The samples taken."""

def __len__(self) -> int:
"""The count of samples."""
return len(self._samples)

def reset(self) -> Self:
"""Reset the samples.

Returns:
Self.
"""
self._samples.clear()
return self

def record(self, value: float, at_time: float | None = None) -> Self:
"""Record a value.

Args:
value: The value to record.
at_time: The time point at which to make the record.

Returns:
Self.

Raises:
ValueError: If the recorded value is out of order or out of bounds.
"""
samples = self._samples

# If the last sample is higher in value than the new one...
if samples and samples[-1].value > value:
# ...treat that as an error.
raise ValueError(f"{value} is less than the previously-recorded value")
# If the sample is higher than the destination...
if value > self._destination:
raise ValueError(
f"{value} is greater than the destination of {self._destination}"
)

# Default to "now" if we didn't get handed a time.
at_time = monotonic() if at_time is None else at_time

# If we seem to be going backwards in time...
if samples and samples[-1].moment > at_time:
# ...treat that as an error.
raise ValueError(f"{at_time} is earlier than the previously-recorded time")

# Record the new sample.
samples.append(Sample(value, at_time))

return self

@property
def _oldest_sample(self) -> Sample | None:
"""The oldest sample we have, if there is one.

For cases where all sample but the most recent one have expired out
of the window, the oldest sample will be the last-dropped sample.
This will help give a best guess as to hold long what's left may
take.
"""
if (samples := len(self._samples)) > 1:
# We have multiple samples, so return the oldest.
return self._samples[0]
elif samples == 1 and (last_dropped := self._samples.last_dropped) is not None:
# We only have the one sample, but we do have a reference to the
# the last-dropped sample, so as a fallback use that.
return last_dropped
# We don't have any samples to go off.
return None

@property
def _elapsed(self) -> float:
"""The time elapsed over the course of the samples.

Note that this is the time elapsed over all of the samples in the
current time/max-count window, or between the last-dropped sample
and the one remaining one in the window if the window is
near-exhausted.
"""
if (oldest := self._oldest_sample) is not None:
return self._samples[-1].moment - oldest.moment
return 0

@property
def _elapsed_to_now(self) -> float:
"""The time elapsed over the course of the samples until now.

Note that this is the time elapsed from the earliest sample in the
current time/max-count window, or from the last-dropped sample
window is near-exhausted (if there's only one sample left in it).

This will always be 0 if no samples have been recorded yet.
"""
if (oldest := self._oldest_sample) is not None:
return monotonic() - oldest.moment
return 0

@property
def _distance_covered_in_window(self) -> float:
"""The distance covered by the samples.

Note that this is just the distance covered by the samples in the
current window; not the distance covered by every sample that has
been recorded.
"""
if (oldest := self._oldest_sample) is not None:
return self._samples[-1].value - oldest.value
return 0

@property
def _distance_remaining(self) -> float:
"""The distance remaining until the destination is reached."""
return self._destination - (self._samples[-1].value if len(self) else 0)

@property
def _speed(self) -> float:
"""The speed based on the recorded samples."""
try:
return self._elapsed / self._distance_covered_in_window
except ZeroDivisionError:
return self._elapsed

@property
def _speed_now(self) -> float:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow the need for a "speed" and a "speed now".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible I could prune this back now, but at least for coding/testing purposes it's been a lot easier to reason about "what's the estimate for the data in the window, as of the last recording point" vs "what's the estimate as of now?"

"""The speed as of right now, based on the recorded samples."""
try:
return self._elapsed_to_now / self._distance_covered_in_window
except ZeroDivisionError:
return self._elapsed_to_now

@property
def estimated_time_to_complete(self) -> float:
"""The estimated time to completion.

This is the time as of the last-recorded sample.
"""
return self._distance_remaining * self._speed

@property
def estimated_time_to_complete_as_of_now(self) -> float:
"""The estimated time to completion as of now."""
return self._distance_remaining * self._speed_now

def __rich_repr__(self) -> Result:
yield "destination", self._destination
yield "estimated_time_to_complete", self.estimated_time_to_complete
yield "estimated_time_to_complete_as_of_now", self.estimated_time_to_complete_as_of_now
yield self._samples


if __name__ == "__main__":
from time import sleep

for portion in range(2, 21):
etc = TimeToCompletion(500)
started = monotonic()
for n in range(500 // portion):
etc.record(n)
sleep(0.01)
elapsed = monotonic() - started
print(f"==== Based on 1/{portion} of the full range ====")
print(f"Elapsed........: {elapsed}")
print(f"ETC............: {etc.estimated_time_to_complete}")
print(f"ETC now........: {etc.estimated_time_to_complete_as_of_now}")
print(f"Estimated total: {etc.estimated_time_to_complete + elapsed}")
Loading
Loading