Skip to content

Make Report.run() the single entry point; deprecate determine_report() / persist_results() #106

Description

@tombonfert

Problem

There are two public ways to execute a report: the one-call Report.run(), and the two-step determine_report() + persist_results(). The skills and narrative docs teach only the two-step flow and never mention run().

This is a correctness gap, not just duplication: run() owns the capped-incremental drain loop. With max_containers_per_batch set on an incremental run, a single determine_report() + persist_results() pass processes only the first batch, so a user following the documented flow silently under-processes the population. run() loops until drained and has a no-forward-progress guard.

Current state

  • run() defined at src/impulse_reporting/core/report.py:945; wraps determine_report() + persist_results() and adds the drain loop.
  • run() documented only in the auto-generated API reference (docs/impulse/docs/references/api/impulse_reporting/core/report.md) and referenced in passing in configuration.md.
  • run() is in no skills/impulse-*/SKILL.md; the impulse-reporting skill teaches determine_report() + persist_results() (including determine_report(is_incremental=True)).

Proposed fix

  1. Establish run(is_incremental=None, persist_results=True, cleanup_temp_tables=None) as the single recommended entry point.
  2. Deprecate the public determine_report() and persist_results(): keep them functional but emit a DeprecationWarning pointing to run(), and keep the underlying logic (which run() already calls internally). Plan a later release to make them private or remove them.
  3. Update skills/impulse-reporting/SKILL.md and the narrative reporting docs to use run() throughout.

Migration notes

  • determine_report() + persist_results() becomes run().
  • Determine-only / sinkless / notebook use (compute results on the report object without writing) maps to run(persist_results=False).
  • Minor wart to keep in mind: run() already has a persist_results bool parameter that shares a name with the method being deprecated; that is fine but worth a doc note.

Acceptance criteria

  • Calling determine_report() or persist_results() emits a DeprecationWarning naming run() as the replacement, with existing behavior unchanged.
  • impulse-reporting skill and reporting docs show run() as the entry point, including run(persist_results=False) for the sinkless case.
  • Wording stays short and consistent with existing skill/doc style.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions