feat(collection): optional progress bar for Collection.upsert (#10) - #118
srijanarya wants to merge 2 commits into
Conversation
…se#10) Adds opt-in show_progress to Collection.upsert. Uses tqdm.auto so it picks a notebook widget or terminal bar; reports a known total when records is Sized, otherwise an indeterminate bar. tqdm is an optional extra (vecs[progress]); default behaviour is unchanged. Scoped to upsert; create_index progress is left for a follow-up.
marcosbeta23
left a comment
There was a problem hiding this comment.
Went through the diff against what we discussed on #10 — solid implementation. Left one inline note about progress-bar cleanup on an exception path; everything else looks good to me.
| sess.execute(stmt) | ||
| if progress is not None: | ||
| progress.update(len(chunk)) | ||
|
|
||
| if progress is not None: | ||
| progress.close() |
There was a problem hiding this comment.
Nice work overall — the show_progress scoping is clean, and tqdm.auto resolves the notebook/terminal split exactly as discussed in the issue.
One correctness note on this hunk: progress.close() (and the update() calls) only run if the per-chunk loop finishes without error. If sess.execute(stmt) raises partway through, the function exits via the exception and the bar never closes — harmless in a plain terminal, but tqdm.auto's notebook widget can be left half-rendered if that happens in Jupyter.
Since the fix needs to wrap the whole per-chunk loop rather than just these lines, something like a try/finally around the loop body (with progress.close() in the finally), or using tqdm as a context manager, would make this exception-safe. Happy to sketch it out further if useful — not blocking, but worth tightening before merge.
Closes #10, scoped to
upsert;create_indexprogress is left for a follow-up as discussed in the issue.Adds an opt-in
show_progress: bool = Falseparameter toCollection.upsert. When enabled:tqdm.auto, so it picks a notebook widget or a terminal bar automatically (the notebook-vs-shell problem raised in the issue).recordsisSized(list, tuple, ...), and falls back to an indeterminate bar for generators and other unsized iterables.tqdmis an optional extra (pip install vecs[progress]), not a new hard dependency, sopip install vecsis unchanged. Calling withshow_progress=Truewithout tqdm installed raises anImportErrorwith the install hint.show_progress=False) is unchanged.Tests added in
src/tests/test_upsert_progress.pyfor: default creates no progress object, sized input reports the total and updates per 500-record chunk, generator input is indeterminate and still updates per chunk, and theImportErrorpath when tqdm is absent. Full suite run locally againstsupabase/postgres:15.1.1.78: 44 passed; the 3test_adapters.pytext-embedding tests fail identically onmainwithout thesentence-transformersextra installed.