Skip to content

refactor(ingestion): reorganize progress capturing (connector declares, runner counts, registry reports)#29763

Draft
ulixius9 wants to merge 13 commits into
port-louisfrom
port-louis-v1
Draft

refactor(ingestion): reorganize progress capturing (connector declares, runner counts, registry reports)#29763
ulixius9 wants to merge 13 commits into
port-louisfrom
port-louis-v1

Conversation

@ulixius9

@ulixius9 ulixius9 commented Jul 6, 2026

Copy link
Copy Markdown
Member

What & why

Reorganizes ingestion-side progress capturing so the responsibilities are clean and the code is easy to read/extend when adding a new connector. Pure reorganization — counting semantics, the SSE payload shape, and the ETA formula are behavior-preserving. Enforces one ownership split:

  • Connector — knows how to query the source and how to calculate totals (denominators). It never counts processed items.
  • Topology runner — counts processed entities for every connector, by default.
  • ProgressRegistry — stores the state, calculates the ETA, and reports it (CLI tree + SSE payload).

How

  • New single-home package metadata/ingestion/progress/ (registry.py, tracking.py, modes.py, _render.py, runner_tracker.py).
  • One knob replaces the old progress_tracking_enabled flag: progress_mode = AUTO | MANUAL | OFF.
    • AUTO (default) — the runner counts from the topology walk; a brand-new connector gets live progress with zero progress code.
    • Tier-1 totals — a connector implements declare_progress_totals(totals) and declares denominators through a denominator-only TotalsDeclarer facade (structurally cannot increment counts). Runner calls it exactly once, at the first non-root node.
    • MANUAL — escape hatch for connectors that count themselves (PowerBI, usage/lineage query sources) via a ManualProgress facade. Runner emits nothing for them → auto XOR manual, double-counting impossible by construction.
  • topology_runner.py progress policy extracted into TopologyProgressTracker + NodeProgress handles; the walk methods now call handles unconditionally (zero progress ifs in the runner).
  • ProgressRegistry absorbs eta_seconds() / render_cli() / sse_payload(); the standalone ProgressReporter is dissolved.
  • Migrations: Snowflake → declare_progress_totals hook; PowerBI + query-parser family → MANUAL.

Behavior changes (only two)

  1. Default-on — every AUTO topology connector now gets runner-counted progress (leaf producers are materialized one scope at a time for exact counts). OFF is the per-connector fuse.
  2. Totals-hook timing — formalized to "first non-root node" (same walk position Snowflake used before).

Everything else — counting, tree shapes, payloads, ETA math, reconciliation — is byte-for-byte preserved.

Testing

  • Full progress-adjacent suite green (2649 passed); remaining failures are pre-existing missing-optional-dependency env gaps (databricks-sqlalchemy, cwl_airflow, etc.), unrelated to progress.
  • New tests: mode facades, runner tracker gating + once-only totals, plain-connector default-on, MANUAL-no-registry, PowerBI parity.
  • make py_format_check exits 0.

Notes for reviewers

  • The first commit (emit run-started progress update and thread total assets ingested) is pre-existing in-flight branch work bundled here.
  • Legacy utils/progress_tracker.py (step-summary singleton) is intentionally kept — independent code path, out of scope.
  • Out of scope / future: emission-based counting model (PowerBI/API under-count fixes), Filtered/Issues/Outcome feeds, server/UI work.

🤖 Generated with Claude Code

ulixius9 and others added 11 commits July 6, 2026 10:31
…sets ingested

Pre-existing in-flight branch work: emit a DISCOVERY progress update at run
start so short runs are visible before the reporting timer's first tick, and
forward totalAssetsIngested through the SSE payload and server tracker.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etadata/ingestion/progress package

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Progress facades

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ee root

The extra open([], label, total) made the active-scope snapshot non-None after
all groups are pruned, contradicting the established snapshot()-is-None behavior
and reintroducing a parallel count path. Restore parity with the original
_declare_progress_groups helper.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssReporter

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…and PowerBI sources

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…icy behind NodeProgress handles

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… progress default-on with AUTO/MANUAL/OFF

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mode

The group-progress helpers on DashboardServiceSource were migrated to the
manual_progress facade, which requires progress_mode=MANUAL. The _BareSource
test double still used the default AUTO mode and raised ProgressModeError.
Declare MANUAL on the double, matching how real PowerBI drives these helpers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re_progress_totals hook

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ANUAL mode

Guards a future dashboard-connector author from calling the group helpers on a
default AUTO source, which would raise ProgressModeError and otherwise double-count
against the runner's own tracking. Addresses the one latent trap from final review.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added Ingestion safe to test Add this label to run secure Github workflows on PRs labels Jul 6, 2026
Comment thread ingestion/Dockerfile.ci Outdated
ulixius9 and others added 2 commits July 6, 2026 15:06
Dockerfile.ci was accidentally swept into this branch with a local build hack
that pinned pip extras to [powerbi,snowflake,pii-processor]. Restore the
canonical ARG-driven install so this file carries no branch-local change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ess_totals hook

MySQL already inherits AUTO processed-counting from the topology runner; add
the Tier-1 declare_progress_totals hook so the run renders %/ETA. MySQL is
effectively single-database, so declare the Database denominator directly and
mark DatabaseSchema reconcilable — the runner fills the schema total from the
live inspector it queries anyway, and DatabaseSchema drives the ETA.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gitar-bot

gitar-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 1 resolved / 1 findings

Reorganizes progress capturing into a modular architecture to enforce clear ownership between connectors, runners, and the registry. Resolved the Dockerfile.ci dependency issue, and all tests passed.

✅ 1 resolved
Bug: Dockerfile.ci hardcodes connector deps, ignoring INGESTION_DEPENDENCY

📄 ingestion/Dockerfile.ci:129-130
ingestion/Dockerfile.ci was changed from RUN pip install ".[${INGESTION_DEPENDENCY}]" (where ARG INGESTION_DEPENDENCY defaults to all) to a hardcoded RUN pip install ".[powerbi,snowflake,pii-processor]". The ARG INGESTION_DEPENDENCY="all" line is still present but is now ignored by the install step, so the CI image ships only powerbi/snowflake/pii-processor extras instead of the full connector set.

Impact: any CI job or unit/integration test that imports another connector's optional dependencies (mysql, postgres, databricks, kafka, etc.) will fail to import in this image, and callers that pass INGESTION_DEPENDENCY as a build arg will silently have it dropped. This change is unrelated to the progress refactor described in the PR summary and looks like a debugging/scoping leftover.

Suggested fix: restore the parametrized install so the default remains all, or, if a temporary scope-down is intentional for this draft branch, add a TODO with a ticket reference and revert before merge.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

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

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant