Skip to content

feat(notifications): determinate pill progress with a progress detail - #1074

Open
SerafiniJose wants to merge 5 commits into
openforis:mainfrom
SerafiniJose:feat/task-progress-detail-v41
Open

SerafiniJose wants to merge 5 commits into
openforis:mainfrom
SerafiniJose:feat/task-progress-detail-v41

Conversation

@SerafiniJose

Copy link
Copy Markdown
Contributor

What

TaskTracker.set_progress(value, detail=None) takes an optional display string for the current progress. The task pill:

  • makes its ring determinate when progress is set (today the ring is always indeterminate, so set_progress has no visible effect);
  • while a detail is set, alternates every 2.5 s between the task title and {pct}% {detail};
  • shows the detail on its own on an indeterminate task (for example, why it is waiting);
  • colors the ring with --pill-fg instead of hard-coded white, which disappeared on the light pill.

The detail never goes into the milestone log, so per-item updates don't flood it. Leaving it out clears the previous one, and set_progress(None) puts the ring back to indeterminate.

Why

A long multi-item job (for example a GEE download with dozens of tiles per layer) can only report through step(), which logs every update, or set_progress(), which the pill ignores.

Demo

demo_apps/solara_progress_app, also in the gallery as "Task Progress". It runs three tasks: named step() phases, a bare set_progress() fraction, and a simulated tiled download that uses a detail (a wait reason on an indeterminate ring, then the layer title alternating with {pct}% {layer} — tile n/40, and set_progress(None) between layers). Catalogs in en/es/fr. No session and no credentials:

./run_solara.sh demo_apps/solara_progress_app/app.py --port 8901

Compatibility

detail is keyword-only in practice with a None default, TrackedTask.progress_detail defaults to None, and _NoopTaskTracker has the same signature. Existing callers don't change.

Tests

Template and notifier tests added. tests/test_solara/test_notifications: 68 passed; test_demo_apps.py: 39 passed; full suite: 1401 passed, 1 skipped.

- TrackedTask.progress_detail + set_progress(value, detail=None);
  value=None resets the task to indeterminate between items
- serialize progressDetail to the Vue props
- pill ring binds to task progress instead of spinning forever, and
  strokes with the pill foreground (was hardcoded white, invisible on
  the light pill)
- pill text alternates every 2.5s between the task title and
  '{pct}% {progress_detail}' when a detail is present; detail-less
  tasks keep the previous behavior unchanged
- document set_progress in the notifications guide
pillHasDetail() required a numeric progress value, so
set_progress(None, detail=...) silently dropped the detail instead of
alternating it with the title on the spinner. Decouple the two:
pillHasDetail() now only checks progressDetail, and pillPct() gates
the leading percentage on progress being known instead.

Also: append progress_detail after TrackedTask's other fields so
positional construction keeps its old order, switch set_progress()'s
docstring to a Google Args: block, note the throttling cost and the
lastStep-suffix/fade behavior in the docs, dedent the doc snippet, and
drop app-specific wording from the tests/docs in favor of neutral
examples.

@dfguerrerom dfguerrerom left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This can't merge yet. Progress has to reach the pill from every place an app reports it from, and today it only does so from a coroutine on the kernel loop.

Blocking: progress must reach the pill from any thread

Under solara run, a tracker call (set_progress, step, update, complete) made from a thread that has no Solara kernel context is lost without an error. The bus updates its own state, but the reactive write goes to Solara's global store, so the pill doesn't change until a later update arrives from the kernel thread. We reproduced it with a real VirtualKernelContext (the kernel_contexts fixture in tests/_kernel_contexts.py).

spatial-risk-module works around this with spawn_in_context and to_thread_in_context. That can't be the answer for pysepal. Every app would need it, and to_thread_in_context leaves the context attached to a pooled thread. In a shared, multi-user app, the next user whose work runs on that thread would send their progress to the first user's pill. This needs fixing inside pysepal. Progress from worker threads is the main reason this API exists, so the fix belongs in this PR.

The "A worker thread" example in docs/guides/solara-notifications.md has the same problem. Inside asyncio.to_thread, notify() and track_task() don't find the bus at all, so the toast is dropped with a warning. Either make the example work or correct it.

These are the cases that must pass:

Where the call comes from Expected
A coroutine on the kernel loop (use_task(prefer_threaded=False)) Reaches the pill (works today)
use_task(prefer_threaded=True) Reaches the pill
asyncio.to_thread, loop.run_in_executor Reaches the pill (fails today)
threading.Thread and ThreadPoolExecutor started by app code Reaches the pill (fails today)
One pool thread used by two kernels, one after the other Each kernel's update reaches only its own pill
A subprocess (ProcessPoolExecutor) Not supported, because the tracker can't be pickled. Document the pattern: the child sends progress through a queue and the parent calls set_progress
Voila and plain Jupyter Still works

What to test:

  • Unit tests built on kernel_contexts. For each thread shape in the table, subscribe to bus.tasks inside the context and assert that the subscriber sees the update made from the worker. Add one test with two contexts sharing a pool thread, where each subscriber sees only its own task.
  • A live check with the demo. Add buttons that run the download through asyncio.to_thread and through a ThreadPoolExecutor, then try them under solara run and under Voila. Two browser tabs at once cover the two-kernel case.

One possible direction: the bus remembers the kernel context, or the event loop, it was created in, and publishes through it, for example with loop.call_soon_threadsafe. That also merges a burst of updates into one, so callers wouldn't have to throttle. Any approach is fine, as long as the cases above pass.

Also before merging

  1. A stale detail hides steps. If a task calls set_progress(None, detail="waiting…") and then step("Started"), the pill keeps showing "waiting…" and never shows the step, because any detail replaces the last step. step() should clear the detail.
  2. The pill can show 100% too early. Math.round turns 0.995 into "100%" while the task is still running. Use Math.floor.
  3. Progress isn't capped. set_progress(1.5) shows "150%". Clamp the value to 0–1 in Python.
  4. Only one task is visible. With several tasks running, the pill shows only the newest one, and set_progress never writes to the log, so the others' progress can't be seen anywhere. spatial-risk-module runs jobs in parallel, so this matters there. Propose a way to handle it: a count on the pill ("+1"), rotating between the running tasks, or a live progress line for each running task in the log.

Not blocking: the template tests check the Vue source as text, so a refactor breaks them while a logic bug can still pass. A browser check of the alternation with scripts/browser_probe.mjs (see docs/guides/browser-testing.md) would catch real regressions. Also check that the pill's width doesn't jump each time the text alternates.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants