[GSoC] Add paginated, filtered run iteration to the core client - #3348
[GSoC] Add paginated, filtered run iteration to the core client#3348Aryan95614 wants to merge 6 commits into
Conversation
…explicit kwargs
- Gate the cursor-paginated path on metadata-service version (>= 2.5.1) with an
X-Limit capability-header check; fall back to legacy listing for older
services and reject server-side filters when the service cannot honor them.
- Generalize pagination to all collection types, not just flow/run listings.
- Sort iter_objects results newest-first by ts_epoch so max_runs returns the
newest runs regardless of the provider's ordering.
- Replace Flow.runs(**kwargs) with explicit keyword-only filters/page_size/
max_runs, and apply positional tags locally so flow.runs("prod") works with
local metadata as well as the service.
- Extract collection listing into private helpers for readability.
- Use mocks instead of fake providers in the run-listing tests; add coverage
for legacy fallback, capability checks, and newest-first ordering.
Follow-up on the paginated listing path: - Explain why _get_object_internal materializes pages into a list: get_object's object-or-list contract must stay stable, the goal here is to relieve server-side pressure, and callers needing to stream large collections go through iter_objects()/_iter_paginated_records (e.g. Flow.runs()). - Document that result ordering is implicitly newest-first (descending ts_epoch) because there is no _order query param yet, in both the service iterator and the base MetadataProvider.iter_objects sort, so the two paths stay consistent.
…tion - Bump _MIN_SERVICE_VERSION_WITH_CURSOR_PAGINATION to 2.6.0 -- the release that ships pagination + filtering (per maintainer). - The paginated get_object path returned [] where the legacy path returned None for a missing (404) collection. Add a raise_on_missing flag threaded through _iter_paginated_records and _legacy_get_collection so get_object keeps legacy's atomic contract: a 404 at any point in the listing (first page, mid-pagination, or the no-X-Limit legacy fallback) resolves to None, never an empty or silently truncated list. Streaming iter_objects is unchanged: a 404 just ends the stream. - Lift get_object's obj_type/sub_type validation guards into a shared _validate_object_query helper and call it from the paginated listing path, so streamed access rejects the same nonsensical combinations as materialized access. - Cover all of the above with tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3348 +/- ##
=========================================
Coverage ? 31.27%
=========================================
Files ? 382
Lines ? 52903
Branches ? 9354
=========================================
Hits ? 16547
Misses ? 35126
Partials ? 1230 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| page_size: Optional[int] = None, | ||
| max_runs: Optional[int] = None, |
There was a problem hiding this comment.
can we also do away with the page_size and max_runs here, or are they required? page size is a metaflow config level toggle already so I see no reason to keep it as a call level arg as well. It is also service-specific, as the return here is an iterator anyway so the caller does not concern with page sizes
There was a problem hiding this comment.
I dropped page_size from the signature entirely rather than just making it private. You're right that it has no use for the local implementation, and runs() hands back an iterator either way, so the caller never really needed it. It comes from METAFLOW_SERVICE_PAGE_SIZE now, which I've documented, validated, and clamped to the service maximum of 500.
I did keep max_runs public, and I'd rather flag that than slip it past you, since @talsperre +1'd removing both. Sakari's point was that max_runs is different from the other two: it works for every provider including local, and failed_runs(max_runs=...) in #3349 is built on it. If you'd still prefer it gone, I'll take it out.
Greptile SummaryThe PR adds provider-level streaming iteration, metadata-service cursor pagination and filtering, and bounded newest-first run listing. It also consolidates service capability checks into a URL-keyed version cache, but that cache remains stale when the endpoint behind an existing URL changes.
Confidence Score: 4/5The PR should not merge until service capability decisions can refresh when the endpoint behind an already-cached URL changes version. The process-lifetime URL cache can continue rejecting supported filtering after an in-place service upgrade or send unsupported requests after an in-place downgrade. Files Needing Attention: metaflow/plugins/metadata_providers/service.py
|
| Filename | Overview |
|---|---|
| metaflow/client/core.py | Adds streaming child construction and bounded run iteration while preserving the no-option legacy path. |
| metaflow/metadata_provider/metadata.py | Introduces the provider iterator fallback, shared query validation, and newest-first materialized ordering. |
| metaflow/metaflow_config.py | Adds validated configuration for metadata-service pagination page size. |
| metaflow/metaflow_config_funcs.py | Adds a reusable positive-integer configuration validator. |
| metaflow/plugins/metadata_providers/service.py | Implements cursor pagination and shared capability caching, but capability results never refresh when a service changes at the same URL. |
| test/unit/test_client_run_listing.py | Thoroughly exercises pagination and cross-URL cache behavior but does not cover upgrades or replacements behind the same URL. |
Reviews (3): Last reviewed commit: "Paginate artifact collections against me..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| @classmethod | ||
| def _can_paginate_collection(cls, sub_type, attempt): | ||
| return sub_type != "self" and attempt is None |
There was a problem hiding this comment.
We should not paginate artifact already. return sub_type not in ("self", "artifact") and attempt is None. The current metadata service does artifact selction based on latest task attempt, but this PR will find the latest occurrence of every artifact.
There was a problem hiding this comment.
Done, with a comment on the line explaining why, so it doesn't get "optimised" back later.
Thank you for tracing this to the SQL. DISTINCT ON (task_id, name) really is a different operation from the legacy latest-attempt filter, and I hadn't spotted that the two paths disagree on retried tasks. I'll open a follow-up on metaflow-service with the latest_attempt CTE. The client-side guard should stay until that ships and is released, since the client can't tell a fixed 2.6 from an unfixed one.
|
Some additional comments as well here |
…ests
Covers every open thread plus talsperre's written review.
Blocker -- artifact collections no longer paginate:
- Service 2.6.0's paginated artifact query selects the newest row per artifact
*name* (SELECT DISTINCT ON (task_id, name)), not the artifacts of the newest
task *attempt*, so a file written only by attempt 0 could surface on a task
whose latest attempt never wrote it. _can_paginate_collection now excludes
"artifact", with a comment explaining why, so artifacts keep the legacy
latest-attempt semantics. The root cause is in the service SQL and needs a
metaflow-service fix; this is the safe client-side guard until then.
Capability gating is now a property of the selected service, not the process:
- Replace the three process-global booleans (_supports_attempt_gets,
_supports_tag_mutation, _supports_cursor_pagination) with one
_service_version_cache keyed by service URL, read through _service_supports().
metadata("service@...") can repoint a live process at another service; the old
single boolean let a 2.5 verdict disable filtering on a 2.6 service, and let a
2.6 verdict send pagination parameters to a 2.5 one. Each call site keeps its
own minimum-version error message, and one version ping now serves all three
checks per service instead of three.
Pagination protocol failures are loud instead of silently wrong:
- The X-Limit capability header is checked on every page, not just the first.
In a rolling deployment page 2 can reach an older instance that ignores the
cursor and filters and answers with a bulk listing; yielding that would
duplicate and unfilter the stream. Falling back to the legacy listing stays
allowed on the first page only, where nothing has been yielded yet.
- A repeated cursor now raises instead of returning. It is a broken exchange,
and returning presented a truncated listing as a complete one.
- iter_objects validates obj_type/sub_type before the capability ping, so an
invalid internal query no longer makes an HTTP request before failing.
Flow.runs() public surface:
- filters -> _filters. The field:operator grammar is a metadata-service detail
with no meaning for local metadata, so it is not part of the public API;
Flow.failed_runs() is the typed accessor built on top of it.
- page_size is removed from the signature entirely rather than made private.
runs() returns an iterator either way, so page size is a transport concern;
it comes from METAFLOW_SERVICE_PAGE_SIZE.
- max_runs stays public: unlike the other two it is meaningful for every
metadata provider, including local.
- Validate _filters with isinstance(..., Mapping) instead of hasattr("items").
The explicit bool check on max_runs stays, now with a comment: bool subclasses
int, so without it max_runs=True would silently mean one run.
- Extract MetaflowObject._child_from_record() so __iter__ and _iter_children
build children the same way.
Config:
- METAFLOW_SERVICE_PAGE_SIZE is documented, validated as a positive integer
(values from the environment arrive as strings), and clamped to the service
maximum of 500 before it is sent.
Tests:
- Converted to the repo's conventions: pytest-mock instead of unittest.mock.
- Tests that did not exercise what their name claimed now do. The max_runs
ordering test and the local tag-filtering test run end to end against a real
Flow and the local provider instead of slicing provider output or mocking
_iter_children. The collection-type test is parametrized over every paginable
collection. The validation test now fails if either _version() or _request()
is called at all.
- New regression tests: service switch rechecks capability, missing capability
header on a later page raises, repeated cursor raises instead of truncating,
artifact collections never paginate, one version ping per service, each
capability keeps its own message, page size is clamped.
- The repeated-cursor test previously asserted silent truncation. Inverted.
- The artifact test asserts only that artifacts take the legacy path. Comparing
paginated and legacy artifact results needs a metaflow-service test, and
claiming that coverage here would be the same false confidence the review
flagged.
|
Thanks for taking the time to write this up properly. The artifact one in particular I would not have found on my own. Pushed a commit that covers all six points and the test feedback. 1. Artifact pagination. Artifacts are excluded from the paginated path now. The root cause is on the service side, so I'll follow up there with the 2. Capability cache. Replaced all three global booleans with a single version cache keyed by service URL, rather than URL-keying only the pagination flag. Each call site keeps its own error message, and one ping per service now covers all three checks. 3. Only the first page was checked. The 4. Repeated cursor. Raises now, separated from the normal "no next cursor" completion. 5. Validation before HTTP. 6. Tests. You were right, several of them weren't checking what their names claimed. The Since your point was that a green CI proved nothing here, I went back and reverted each fix one at a time to check the new tests actually fail: the artifact exclusion, the per-page header check, the repeated-cursor raise, and the URL-keyed cache each break exactly the test meant to catch them. The full unit suite has the same failures before and after my commit. Two things I didn't quietly decide on my own:
I've also fixed the 2.5.1 / 2.6.0 mismatch in the description and added a short future work note about the provider-translated filter grammar. #3349 is stacked on this, so I've left it alone for now and will update it once this settles. |
metaflow-service 2.6.0 paginated artifacts as the newest row per artifact name rather than the artifacts of the task's latest attempt, so the client kept artifact collections on the legacy bulk GET. 2.6.1 ships the fix (metaflow-service#497), so the artifact exclusion goes away and the pagination gate moves from 2.6.0 to 2.6.1 instead. A 2.6.0 service now falls back to legacy listing for every collection, which is what it shipped with; the per-attempt artifact endpoint still has no cursor pagination and keeps the bulk GET.
| url = cls.INFO | ||
| if url not in cls._service_version_cache: | ||
| cls._service_version_cache[url] = cls._version(None) | ||
| version = cls._service_version_cache[url] |
There was a problem hiding this comment.
If a metadata service is upgraded, downgraded, or replaced without changing its URL, _service_supports continues using the first version cached for that URL. The client then rejects supported filtering and uses legacy listing after an upgrade, or sends unsupported paginated and attempt-specific requests after a downgrade.
Knowledge Base Used:
3 participants
GSoC 2026 project: metadata service request improvements (mentor: @saikonen). Upstream twin of the fork review PR (Aryan95614#1), opened per mentor request for a static PR number.
Summary
Moves cursor pagination and server-side run filtering into the regular Metaflow client instead of maintaining a parallel client stack.
iter_objects()path with a backwards-compatible fallbackX-Limitcapability-header check, falling back to legacy listing for older services and rejecting server-side filters when the service cannot honor themflow/runlistingsX-Next-Cursorfield:operatorfiltersFlow.runs()with explicit keyword-onlyfilters,page_size, andmax_runsFlow.runs("prod")works with local metadata as well as the servicets_epoch, somax_runsreturns the newest runs regardless of provider orderFlow.runs(*tags)behaviorSeparation
This is the base PR. It contains pagination and filtering only. The agent-oriented failure investigation utility is isolated in stacked PR #2.
This structure follows the review direction that pagination, filtering, and normal record iteration belong in the core client.
Verification
black(repo-pinned 25.12.0) clean on the changed filesinheritance/spinintegration failures are environmental and unrelated)Note on the version gate
The paginated/filtered path is gated on metadata-service
>= 2.6.1— the release that ships pagination + filtering together with the artifact latest-attempt fix (metaflow-service#497) — plus anX-Limitcapability-header check as defense-in-depth. Services without it fall back to legacy listing, and server-side filters raise a clear error rather than silently returning unfiltered results.Public surface
Flow.runs()takes*tags,_filters, andmax_runs._filtersis intentionally private: thefield:operatorgrammar belongs to the metadata service and has no meaning for the local provider, so it should not be something callers depend on.page_sizeis not part of the signature at all —runs()returns an iterator either way, so page size is a transport concern and comes fromMETAFLOW_SERVICE_PAGE_SIZE.max_runsstays public because, unlike the other two, it is meaningful for every metadata provider.Future work (not in this PR)
Filters could be expressed in plain python and translated by each provider, so the local metadata provider could support the same calls:
with each provider owning a
filter_funcs_mappingthat matches its internals. Keeping_filtersprivate now means that can land later without a breaking change. Credit for the idea goes to @saikonen.