Skip to content

[GSoC] Add failure-investigation accessors to the core client (stacked on #3348) - #3349

Draft
Aryan95614 wants to merge 6 commits into
Netflix:masterfrom
Aryan95614:agent-client-mentor-showcase
Draft

[GSoC] Add failure-investigation accessors to the core client (stacked on #3348)#3349
Aryan95614 wants to merge 6 commits into
Netflix:masterfrom
Aryan95614:agent-client-mentor-showcase

Conversation

@Aryan95614

Copy link
Copy Markdown

GSoC 2026 project: metadata service request improvements (mentor: @saikonen). Upstream twin of the fork review PR (Aryan95614#2), opened per mentor request for a static PR number.

Stacked on #3348 — until that merges, this diff includes its commits; only the top two commits (Fold failure investigation into the core client + the docstring fix) are this PR's own.

Summary

Folds bounded failure investigation into the regular Metaflow client as three
accessors on the existing objects, instead of a separate metaflow/agent
module. Built on the core pagination/filtering from PR #1.

Context / Motivation

Review on the earlier revision steered the run-listing and pagination pieces
into the core client (that's PR #1). Following the same direction, the failure
investigation layer now lives on the client objects rather than a parallel
metaflow.agent package, so there's a single, discoverable API.

Changes Made

  • Flow.failed_runs(*, since=None, max_runs=None) -> Iterator[Run] — failed
    runs newest-first, via runs(filters={"status:eq": "failed"}); since adds
    an inclusive ts_epoch:ge bound.
  • Run.failed_task -> Optional[Task] — the first unsuccessful task in the run
    (newest step first), or None.
  • Task.failure_summary -> Optional[FailureSummary] — normalized
    {exception_type, message, stacktrace, attempt}, or None when the task has
    no exception. Read errors propagate rather than being swallowed.
  • Removed the metaflow/agent module (failure_finder, pagination,
    run_lister, TaskRef/FailureInfo, injectable resolver/fetcher).

Behavior note: failed_runs relies on server-side filtering, so it raises
against the local metadata provider or a service without pagination/filtering
support — the same gate as runs(filters=...).

Testing

  • test/unit/test_client_failure_investigation.py — 14 tests covering
    failed_runs filter/bounds forwarding, failed_task iteration order,
    failure_summary construction + error propagation, and _normalize_exception
    across mapping/object/string inputs.
  • Combined with PR Support for the R programming language #1's run-listing tests: 33 passed.
  • black (repo-pinned 25.12.0) clean.

Trade-offs / Design Decisions

  • Dropped the injectable task_resolver / artifact_fetcher and the TaskRef
    indirection — unnecessary once this is core; real Task objects are used and
    the methods are testable with mocks.
  • Kept FailureSummary as a small frozen dataclass in core.py rather than
    returning a bare dict, so the normalized fields are typed and discoverable.

…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.
Replaces the separate metaflow/agent module with core-client accessors, per
the review that record iteration and normal client operations belong in core:

- Flow.failed_runs(*, since=None, max_runs=None): iterate failed runs newest
  first via runs(filters={"status:eq": "failed"}); raises on local/older
  services like any other server-filtered listing.
- Run.failed_task: the first unsuccessful task in the run (newest step first).
- Task.failure_summary -> FailureSummary: normalized exception type/message/
  stacktrace/attempt, or None when the task has no exception. Read errors
  propagate rather than being swallowed into diagnostic strings.

Drops the injectable resolver/fetcher and the TaskRef indirection (uses real
Task objects). Adds test/unit/test_client_failure_investigation.py.
@saikonen
saikonen requested review from saikonen and talsperre August 23, 2026 23:06
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.79675% with 62 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@4fce948). Learn more about missing BASE report.

Files with missing lines Patch % Lines
metaflow/plugins/metadata_providers/service.py 81.81% 14 Missing and 12 partials ⚠️
metaflow/client/core.py 71.21% 13 Missing and 6 partials ⚠️
metaflow/metadata_provider/metadata.py 52.77% 10 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3349   +/-   ##
=========================================
  Coverage          ?   31.14%           
=========================================
  Files             ?      381           
  Lines             ?    52869           
  Branches          ?     9348           
=========================================
  Hits              ?    16468           
  Misses            ?    35170           
  Partials          ?     1231           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant