Repository navigation
feat(reporting): make Report.run() the single entry point, deprecate determine_report() and persist_results() - #122
Merged
Conversation
…n favor of `run()` - Mark `Report.determine_report()` and `Report.persist_results()` as deprecated with `FutureWarning`; keep behavior unchanged. - Refactor `Report.run()` to call internal `_determine_report()`/`_persist_results()` so the public two-step API can warn independently. - Update README, demos, docs, and skills to use `run()` (and `run(persist_results=False)` for compute-only/sinkless workflows). - Migrate tests to `run()` or the internal helpers where deprecation warnings are not the subject under test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
+ Coverage 90.21% 90.22% +0.01%
==========================================
Files 66 66
Lines 6009 6017 +8
Branches 757 757
==========================================
+ Hits 5421 5429 +8
Misses 466 466
Partials 122 122
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ix doc typos - Clarify that `run(persist_results=False)` leaves lazy DataFrames on the report object, which are unsafe to read after a persisting run because incremental results re-detect against updated gold, capped runs keep only the last batch, and `cleanup_temp_tables` drops their backing temp tables. - Update API reference, report reference, and skill docs to steer users toward reading gold tables after persistence. - Fix misplaced admonition text in the report reference and tighten deprecated-method docstrings. - Adjust integration tests to use internal `_determine_report()`/`_persist_results()` where the test subject is persistence behavior, not the deprecated public API.
…un() telemetry test - Shift `@telemetry_logger` from deprecated public `persist_results()`/`determine_report()` to their private `_persist_results()`/`_determine_report()` counterparts so `run()` logs per-step tags correctly. - Adjust deprecation warning `stacklevel` from 3 to 2 now that the wrappers no longer sit between the caller and the warning. - Add `TestRunTelemetry` verifying that `run()` emits `report/run`, `report/determine_report`, and `report/persist_results` telemetry tags.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #106.
Report.run()is now the one documented way to execute a report. The two-stepdetermine_report()+persist_results()flow still works but emits aFutureWarningpointing torun(). This matters for correctness: the drain loop formax_containers_per_batchlives only inrun(). A capped incremental run following the old documented flow silently processed just the first batch.Changes
_determine_report()/_persist_results(), andrun()now calls those. The public methods are thin wrappers that warn and then delegate, so behavior is unchanged. They useFutureWarningrather thanDeprecationWarning, matching the existingbatch_sizedeprecation:DeprecationWarningis hidden by default when raised from library code.run()now sends onerunping per call instead of extra pings on every iteration. Also removed a mislabeleddetermine_reporttag on_persist_incremental.run(),run(persist_results=False), or the private steps where a test needs a single step or a single capped pass. Added tests for the warning, that it points at the caller, and that the wrappers delegate.run(). They coverrun(persist_results=False)for sinkless or compute-only use and include a deprecation note.Migration
determine_report()+persist_results()run()determine_report()onlyrun(persist_results=False)