Skip to content

Commit 8cfad37

Browse files
authored
OP#10282: Show a progressbar while track files are parsed
Parsing moved off the event loop, so the click that starts loading returns long before the tracks are in the repository. Nothing in the browser said so: no progressbar, no disabled buttons, nothing. A user opening a large otconfig saw an idle looking application with live buttons that silently did nothing, and the acceptance tests, which slept a fixed five seconds and then carried on, clicked "Export track statistics ..." into an application without tracks. LoadTrackFiles already took a ProgressbarBuilder but never used it. Work handed to a worker thread cannot be tracked by decorating a sequence, so builders now also start a progressbar the caller closes again. Builders that can only decorate a sequence show nothing, which is what a command line run wants. The acceptance helpers wait for that progressbar to disappear instead of sleeping. While there, poll for the exported file rather than sleeping five seconds and asserting it exists: exporting takes about four seconds here, so the wait was already marginal and failed on a busy machine.
1 parent b730dab commit 8cfad37

7 files changed

Lines changed: 287 additions & 16 deletions

File tree

‎OTAnalytics/application/use_cases/load_track_files.py‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,13 @@
55
from OTAnalytics.application.logger import logger
66
from OTAnalytics.application.parser.track_parser import TrackParser, TracksParseResult
77
from OTAnalytics.application.state import TracksMetadata, VideosMetadata
8-
from OTAnalytics.domain.progress import ProgressbarBuilder
8+
from OTAnalytics.domain.progress import ProgressbarBuilder, RunningProgressbar
99
from OTAnalytics.domain.track_repository import TrackFileRepository, TrackRepository
1010
from OTAnalytics.domain.video import VideoRepository
1111

12+
PARSING_DESCRIPTION = "Parsing track files"
13+
PARSING_UNIT = "files"
14+
1215

1316
class LoadTrackFiles:
1417
def __init__(
@@ -51,7 +54,11 @@ def __call__(self, files: list[Path]) -> None:
5154
" Use load() instead."
5255
)
5356
if files_to_load := self._files_to_load(files):
54-
self._publish(self._parse(files_to_load), files_to_load)
57+
progressbar = self._start_progress(files_to_load)
58+
try:
59+
self._publish(self._parse(files_to_load), files_to_load)
60+
finally:
61+
progressbar.close()
5562

5663
async def load(self, files: list[Path]) -> None:
5764
"""Load and parse track files together with their videos.
@@ -63,11 +70,32 @@ async def load(self, files: list[Path]) -> None:
6370
files (list[Path]): files in ottrk format.
6471
"""
6572
if files_to_load := self._files_to_load(files):
66-
# parse_files must stay pure: no repository, no observer, no ui. It runs
67-
# on a worker thread here, and repositories notify observers that mutate
68-
# widgets, which is only safe on the event loop.
69-
parse_result = await asyncio.to_thread(self._parse, files_to_load)
70-
self._publish(parse_result, files_to_load)
73+
progressbar = self._start_progress(files_to_load)
74+
try:
75+
# parse_files must stay pure: no repository, no observer, no ui. It
76+
# runs on a worker thread here, and repositories notify observers
77+
# that mutate widgets, which is only safe on the event loop.
78+
parse_result = await asyncio.to_thread(self._parse, files_to_load)
79+
self._publish(parse_result, files_to_load)
80+
finally:
81+
progressbar.close()
82+
83+
def _start_progress(self, files_to_load: list[Path]) -> RunningProgressbar:
84+
"""Show that track files are being parsed until the caller closes it again.
85+
86+
The parser reports nothing until it is done with all of the files, so this
87+
counts no progress. It says what is keeping the application busy, and it
88+
goes away once the files are loaded.
89+
90+
Args:
91+
files_to_load (list[Path]): the files about to be parsed.
92+
93+
Returns:
94+
RunningProgressbar: the progressbar to close once the files are loaded.
95+
"""
96+
return self._progressbar.start(
97+
PARSING_DESCRIPTION, PARSING_UNIT, len(files_to_load)
98+
)
7199

72100
@staticmethod
73101
def _event_loop_is_running() -> bool:

‎OTAnalytics/domain/progress.py‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,27 @@ def __iter__(self) -> Iterator:
4141
raise NotImplementedError
4242

4343

44+
class RunningProgressbar(ABC):
45+
"""A progressbar shown for as long as the caller keeps it open.
46+
47+
Work handed to a worker thread cannot be tracked by iterating a sequence,
48+
because only the thread doing the work would advance it. Such a caller opens
49+
a progressbar, does the work, and closes it again.
50+
"""
51+
52+
@abstractmethod
53+
def close(self) -> None:
54+
"""Stop showing the progress."""
55+
raise NotImplementedError
56+
57+
58+
class NoRunningProgressbar(RunningProgressbar):
59+
"""A progressbar showing nothing, for builders that only decorate sequences."""
60+
61+
def close(self) -> None:
62+
pass
63+
64+
4465
class ProgressbarBuilder(ABC):
4566
"""Interface defining a Progressbar builder.
4667
@@ -62,6 +83,22 @@ def __call__(self, sequence: Sequence, description: str, unit: str) -> Iterable:
6283
"""
6384
raise NotImplementedError
6485

86+
def start(self, description: str, unit: str, total: int) -> RunningProgressbar:
87+
"""Show progress of work that is not driven by iterating a sequence.
88+
89+
Builders that can only decorate a sequence show nothing, which is what a
90+
command line run wants: its output is already a log.
91+
92+
Args:
93+
description (str): what the operation being tracked is doing.
94+
unit (str): the unit of the counted items.
95+
total (int): the number of items expected to complete.
96+
97+
Returns:
98+
RunningProgressbar: the progressbar to close once the work is done.
99+
"""
100+
return NoRunningProgressbar()
101+
65102

66103
class NoProgressbarBuilder(ProgressbarBuilder):
67104
def __call__(self, sequence: Sequence, description: str, unit: str) -> Iterable:

‎OTAnalytics/plugin_ui/nicegui_gui/nicegui/progressbar.py‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,15 @@
1212
GeneralKeys,
1313
ResourceManager,
1414
)
15-
from OTAnalytics.domain.progress import Counter, ProgressbarBuilder
15+
from OTAnalytics.domain.progress import Counter, ProgressbarBuilder, RunningProgressbar
1616
from OTAnalytics.plugin_ui.nicegui_gui.test_constants import TEST_ID
1717

1818
MARKER_PROGRESSBAR_CANCEL = "progressbar-cancel"
1919
MARKER_MESSAGE = "progressbar-message"
2020
MARKER_CURRENT_ITEM = "progressbar-current-item"
2121

2222

23-
class NiceguiProgressbar:
23+
class NiceguiProgressbar(RunningProgressbar):
2424
"""A determinate progressbar rendered in the browser.
2525
2626
Shows how many of the expected items are done, which item was completed last,
@@ -154,6 +154,19 @@ def progressbar(self) -> NiceguiProgressbar:
154154
raise ValueError("No progressbar has been built yet")
155155
return self._progressbar
156156

157+
def start(self, description: str, unit: str, total: int) -> NiceguiProgressbar:
158+
"""Show a progressbar until the caller closes it again.
159+
160+
Args:
161+
description (str): what the operation being tracked is doing.
162+
unit (str): the unit of the counted items.
163+
total (int): the number of items expected to complete.
164+
165+
Returns:
166+
NiceguiProgressbar: the opened progressbar.
167+
"""
168+
return self.build(description, unit, total)
169+
157170
def build(self, description: str, unit: str, total: int) -> NiceguiProgressbar:
158171
"""Build and show a progressbar advanced by reporting completions.
159172

‎tests/acceptance/conftest.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@
2222
PLAYWRIGHT_QUICK_VISIBLE_TIMEOUT_MS = 1000
2323
# Short one-off UI settle wait (milliseconds)
2424
PLAYWRIGHT_SHORT_WAIT_MS = 150
25+
# Parsing track files runs off the event loop and may take a while on a cold
26+
# feather cache, so waiting it out needs far more headroom than the other ui waits
27+
TRACK_LOADING_TIMEOUT_MS = 120_000
28+
# Exports take seconds rather than milliseconds, and more of them on a busy
29+
# machine, so give them room instead of guessing a sleep
30+
EXPORT_TIMEOUT_MS = 120_000
2531
# Polling intervals (milliseconds)
2632
PLAYWRIGHT_POLL_INTERVAL_MS = 50
2733
PLAYWRIGHT_POLL_INTERVAL_SLOW_MS = 100

‎tests/unit/OTAnalytics/application/use_cases/test_load_track_files.py‎

Lines changed: 119 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,11 @@
1212
TrackParser,
1313
TracksParseResult,
1414
)
15-
from OTAnalytics.application.use_cases.load_track_files import LoadTrackFiles
15+
from OTAnalytics.application.use_cases.load_track_files import (
16+
PARSING_DESCRIPTION,
17+
PARSING_UNIT,
18+
LoadTrackFiles,
19+
)
1620
from OTAnalytics.domain.georeference import GeoreferenceMetadata
1721
from OTAnalytics.domain.track import TrackId
1822
from OTAnalytics.domain.track_dataset.track_dataset import (
@@ -416,6 +420,119 @@ def record_publishes_so_far(files: list[Path]) -> Any:
416420
given.track_repository.add_all.assert_called_once()
417421

418422

423+
class TestLoadTrackFilesShowsProgress:
424+
"""#Requirement https://openproject.platomo.de/wp/10282"""
425+
426+
def test_blocking_call_shows_a_progressbar_until_the_files_are_loaded(
427+
self,
428+
) -> None:
429+
given = setup(
430+
track_ids=[TrackId("1"), TrackId("2")],
431+
video_files=[Path("video1.mp4"), Path("video2.mp4")],
432+
track_files=[some_file, other_file],
433+
existing_track_files=[],
434+
classes={"class1"},
435+
)
436+
target = create_target(given)
437+
438+
target([some_file, other_file])
439+
440+
given.progressbar.start.assert_called_once_with(
441+
PARSING_DESCRIPTION, PARSING_UNIT, 2
442+
)
443+
given.progressbar.start.return_value.close.assert_called_once_with()
444+
445+
async def test_shows_a_progressbar_until_the_files_are_loaded(self) -> None:
446+
"""Parsing off the event loop leaves the ui free to show what it is doing."""
447+
given = setup(
448+
track_ids=[TrackId("1")],
449+
video_files=[Path("video1.mp4")],
450+
track_files=[some_file],
451+
existing_track_files=[],
452+
classes={"class1"},
453+
)
454+
target = create_target(given)
455+
456+
await target.load([some_file])
457+
458+
given.progressbar.start.assert_called_once_with(
459+
PARSING_DESCRIPTION, PARSING_UNIT, 1
460+
)
461+
given.progressbar.start.return_value.close.assert_called_once_with()
462+
463+
async def test_counts_only_the_files_it_is_going_to_parse(self) -> None:
464+
given = setup(
465+
track_ids=[TrackId("1")],
466+
video_files=[Path("video1.mp4")],
467+
track_files=[other_file],
468+
existing_track_files=[some_file],
469+
classes={"class1"},
470+
)
471+
target = create_target(given)
472+
473+
await target.load([some_file, other_file])
474+
475+
given.progressbar.start.assert_called_once_with(
476+
PARSING_DESCRIPTION, PARSING_UNIT, 1
477+
)
478+
479+
async def test_opens_the_progressbar_before_parsing_and_closes_it_after(
480+
self,
481+
) -> None:
482+
given = setup(
483+
track_ids=[TrackId("1")],
484+
video_files=[Path("video1.mp4")],
485+
track_files=[some_file],
486+
existing_track_files=[],
487+
classes={"class1"},
488+
)
489+
target = create_target(given)
490+
491+
await target.load([some_file])
492+
493+
assert call_names(given) == [
494+
"progressbar.start",
495+
"track_parser.parse_files",
496+
"videos_metadata.update",
497+
"video_parser.parse",
498+
"video_repository.add_all",
499+
"track_repository.add_all",
500+
"tracks_metadata.update_detection_classes",
501+
"progressbar.start().close",
502+
]
503+
504+
async def test_closes_the_progressbar_when_parsing_fails(self) -> None:
505+
"""A progressbar left open would hide the ui behind it for good."""
506+
given = setup(
507+
track_ids=[TrackId("1")],
508+
video_files=[Path("video1.mp4")],
509+
track_files=[some_file],
510+
existing_track_files=[],
511+
classes={"class1"},
512+
)
513+
given.track_parser.parse_files.side_effect = ValueError("broken track file")
514+
target = create_target(given)
515+
516+
with pytest.raises(ValueError):
517+
await target.load([some_file])
518+
519+
given.progressbar.start.return_value.close.assert_called_once_with()
520+
521+
async def test_shows_nothing_when_every_file_is_loaded_already(self) -> None:
522+
given = setup(
523+
track_ids=[],
524+
video_files=[],
525+
track_files=[],
526+
existing_track_files=[some_file],
527+
classes=set(),
528+
)
529+
target = create_target(given)
530+
531+
await target.load([some_file])
532+
533+
given.progressbar.start.assert_not_called()
534+
535+
419536
def call_names(given: "Given") -> list[str]:
420537
"""The collaborator methods called, in call order."""
421538
return [name for name, _, _ in given.order.mock_calls]
@@ -451,6 +568,7 @@ def __post_init__(self) -> None:
451568
self.order.track_repository = self.track_repository
452569
self.order.video_parser = self.video_parser
453570
self.order.tracks_metadata = self.tracks_metadata
571+
self.order.progressbar = self.progressbar
454572

455573

456574
def setup(

‎tests/unit/OTAnalytics/plugin_ui/nicegui_gui/nicegui/test_progressbar.py‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,31 @@ def page() -> None:
180180
assert progressbars[1].is_open is True
181181

182182

183+
class TestProgressbarForWorkOffTheEventLoop:
184+
"""#Requirement https://openproject.platomo.de/wp/10282"""
185+
186+
@pytest.mark.asyncio
187+
async def test_stays_open_until_the_caller_closes_it(
188+
self, user: User, resource_manager: ResourceManager
189+
) -> None:
190+
"""Parsing on a worker thread reports nothing until it is done."""
191+
given = create_given(resource_manager)
192+
target = create_builder(given)
193+
progressbars = []
194+
195+
@ui.page("/test")
196+
def page() -> None:
197+
progressbars.append(target.start("Parsing track files", "files", 2))
198+
199+
await user.open("/test")
200+
await user.should_see("Parsing track files 0 / 2 files")
201+
assert progressbars[0].is_open is True
202+
203+
progressbars[0].close()
204+
205+
assert progressbars[0].is_open is False
206+
207+
183208
class TestProgressbarWithoutABrowser:
184209

185210
@pytest.mark.asyncio

0 commit comments

Comments
 (0)