Feat/cursor paginationAdd cursor pagination for flow, task, metadata, and artifact endpoints#488
Feat/cursor paginationAdd cursor pagination for flow, task, metadata, and artifact endpoints#488guts27 wants to merge 25 commits into
Conversation
…igration version registration
…d, drop unordered option in test helper
saikonen
left a comment
There was a problem hiding this comment.
some conflicts to resolve due to merging other work to master, otherwise looking good.
| raise ValueError("invalid_cursor") | ||
|
|
||
| if "ts_epoch" not in decoded or "run_number" not in decoded: | ||
| if "ts_epoch" not in decoded: |
There was a problem hiding this comment.
is there a way to nicely include the known cursor-fields, for example in the databaseTable classes, and then pass these to this helper?
Not necessary to tackle in this PR, but a possible part for improving later.
There was a problem hiding this comment.
That's a good point! If each table has its own cursor fields (like a cursor_keys class attribute) and passes them to decode_cursor, the validation will be clear and easy to reuse.
As you suggested, I will leave it out of this PR. But I'd be happy to work on it after this PR is merged.
I will rebase on master to fix the conflicts now.
There was a problem hiding this comment.
Of the 3 CI failures, here's how I handled the artifact ordering issue in the
ui_backend, plus one open question on the metadata test.
I initially tried adding a fallback in find_records so it would use the table's ordering when no explicit order was given. This fixed the artifact ordering, but caused test_task_attempts_with_attempt_metadata to fail intermittently (a duration value being compared against ~0 via pytest.approx). I couldn't fully explain what was causing it. The task handler already passes initial_order=["attempt_id DESC"], so the task query itself shouldn't be affected by the fallback. My best guess is that the fallback slightly shifted the overall test execution timing, and that test's duration assertion is timing-sensitive — but I couldn't confirm this. Since I couldn't pinpoint the root cause, I dropped the global fallback and instead followed the existing pattern in task.py: passing initial_order explicitly in the artifact handlers. This keeps the change scoped to artifacts and leaves task queries untouched. All tests now pass.
test_metadata_post assumed insertion order (A, then B), but metadata was never explicitly ordered — neither the table nor the handler sets a sort order. So the order was incidental, and my new index changed that order. Two options to resolve: 1. Add ordering to get_metadata matching the pagination path (get_metadata_paginated already uses ts_epoch DESC, id DESC), and update the test to expect that order. This keeps the paginated and non-paginated paths consistent. 2. Relax the test to be order-independent, since metadata order was never guaranteed. I'm leaning toward Option 1 for consistency with the cursor pagination. But since metadata ordering was never formally defined, I wanted to check if there's a preferred order — or if it doesn't really matter for the UI. Any feedback would be appreciated!
There was a problem hiding this comment.
The metadata ordering should not matter for existing UI or Metaflow client, so either approach should be fine. I'd maybe lean towards option 1 as well, just so that we are being explicit about the order.
There was a problem hiding this comment.
also as a sidenote, some of the tests that depend on time from Postgres side can be slightly flaky, so rerunning the test cases for those before any changes is a good check as well. (anything status / finished_at related)
There was a problem hiding this comment.
Applied option 1 — added explicit ts_epoch DESC, id DESC ordering to
get_metadata to match the paginated path. All CI checks pass now.
| "400": | ||
| description: invalid cursor |
There was a problem hiding this comment.
duplicate 400 error in docstring
| ) | ||
|
|
||
| db_response, pagination = await self.execute_sql( | ||
| select_sql=select_sql, values=values, limit=limit |
There was a problem hiding this comment.
does this still need limit passed as well? it is part of the outer SQL template
There was a problem hiding this comment.
Good catch. I dropped ir. Pagination.limit is overwritten right after
the call and cur_limit is already in the SQL template.
Summary
Adds non-breaking cursor pagination to the flow, task, metadata, and artifact endpoints, using the same pattern as runs (#482).
Changes
get_records_paginatedPerformance (profiled with EXPLAIN ANALYZE, ~100k rows)
The indexes turn Seq Scan + Sort into an Index Scan, removing the sort node entirely.
Design notes / questions
flow uses
(ts_epoch, flow_id)— no filter columns since it lists all flows,unlike task/metadata which filter by flow_id/run_number first
Artifact index excludes
step_name: including it breaks by_run queries (which don't filter on step_name), pushing them from 20ms back to 245ms.Omitting it keeps all three endpoints (run/step/task) covered.
Cursor tie-breaker uses
(ts_epoch, task_id, name): parallel foreach tasks can share the samets_epoch, sonameis needed to sort them properly when ts_epoch is the same.Verified on the local large dataset.
Backward-compat: the artifact list handlers now use
@format_responseto return the cursor header. For the non-200 path, I kept the originalhttp_500response shape so existing clients aren't affected.Please let me know if this is the right approach.
Migrations use
CREATE INDEX CONCURRENTLYto avoid locking on large tables.Notes
Production scale and data distribution may differ, but the relative improvement (Seq Scan + Sort → Index Scan) should hold.