refactor!: delegate notifications to ProcessExecutionReport - #6
Merged
Merged
Conversation
…k channels Simplify usage at the cost of flexibility: a Task no longer sends notifications. Error notification is delegated entirely to ProcessExecutionReport.notify (whose only_errors / tasks kwargs already provide the granularity). A Task keeps its logfile. Domain: - Task drops the `channels` parameter; gains `traced_vars_frame_filter` (the capture-time knob, formerly sourced from a channel). Capture is single-source in Task.run and feeds both the report and the logfile, so the filter belongs on Task, not on a channel. - Move the logfile formatter into the domain (`_logfile.py`); Task builds its FileHandler inline. The domain no longer imports `comms` at all. comms becomes a pure report-delivery package: - Remove NotificationChannel, _FileChannel, and the streaming email/webhook formatters + handlers (_HTMLEmailFormatter/_HTMLEmailHandler, _WebhookFormatter/_WebhookHandler). - EmailChannel / WebhookChannel are now ReportChannel-only. - HTMLEmailStyle drops `style` and `traced_vars_frame_filter` (reports honor palette + language only); the classic/modern/compact templates are deleted. Tests: drop the streaming-channel suites; repoint render coverage to the report renderer and frame-filter capture via Task; trim the complex-DAG test to its DAG outcomes; remove the obsolete streaming manual scripts. BREAKING CHANGE: Task(channels=...) and the NotificationChannel API are removed; HTMLEmailStyle.style and traced_vars_frame_filter are removed (the latter moves to Task). Per-task email/webhook alerts are replaced by report.notify.
- README + docs/index + docs/examples/advanced: notifications are delivered via report.notify(EmailChannel/WebhookChannel, only_errors=, tasks=) instead of per-task channels; HTMLEmailStyle is palette+language only; traced_vars_frame_filter documented on Task; add ReportContent reference - Remove arquitectura.md, report-notifications-design.md and report-notifications-implementation.md (point-in-time design scratch)
Add `slots=True` to the immutable value types ErrorData, TaskReportEntry, ProcessExecutionReport, ReportContent, and HTMLEmailStyle. The multi-instance types (one TaskReportEntry per task, one ErrorData per failure) gain lower per-instance memory and faster attribute access; for all of them slots also locks the object shape, reinforcing that they are closed, immutable values. Mutable, side-effectful classes (Task, Process, ProcessRunner) are left unchanged on purpose.
Task names and the dependency references that point at them are now lowercased at construction (Task.name and TaskDependency.task_name), with a str type-check before normalizing. This makes duplicate-name detection and dependency resolution case-insensitive and aligns them with the already case-insensitive notify(tasks=...) filter, closing the asymmetry where Fetch and fetch were distinct in the graph but merged by the report filter. additional_kwarg_name is left untouched — it is the real callable parameter name, not a task reference. Tests covering the graph by name are updated to the normalized casing, plus a new TestNameNormalization covering lowercasing, case-insensitive duplicate detection, and cross-case dependency resolution.
Remove 7 keys that no renderer reads (verified: no dynamic key construction) from all 6 language files: - lang_title_prefix, lang_failure_header, lang_failure_header_short, lang_email_subject -- leftovers from the removed per-task email path - lang_args_label, lang_kwargs_label -- the report never labels args/kwargs - lang_downstream_blurb -- the downstream section lists tasks under the title with no blurb sentence Each file goes from 25 to 18 keys (42 redundant entries removed total).
Decorate _load_language_strings with functools.cache so each bundled translation JSON is read from disk at most once per language for the life of the process, instead of on every render. Callers only read the mapping (never mutate it), so sharing the cached dict is safe. This removes the redundant double read per email: the strings are needed both for the HTML body (_build_report_html) and for the subject line (send_report_email), which are independently callable; the cache lets each stay self-contained without paying a second disk read.
Extract the per-render disk reads in _build_report_html into cached loaders, mirroring _load_language_strings: - _load_palette_css(palette): one read per palette (4 possible) per process - _load_report_template(): the report.html template is a single invariant file, previously re-read on every render Both return immutable strings; the template is composed with str.replace (which yields a new string), so the cached values are never mutated. A render now performs zero disk reads once each asset has been loaded once.
Iterate languages (en/es), palettes (all 4), content styles (full/trace/min), and only_errors modes in tests/manual_tests/manual_report_notify.py, sending one email per combination (48 total) to a distinct recipient encoding the combo.
Process now accepts an optional name, carried on ProcessExecutionReport as process_name. The email subject is decorated with the process name (when set) and the run date (YYYYMMDD); the webhook payload always emits process_name alongside entries for a stable schema.
oliverm91
changed the title
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
Replaces the old per-task notification model with a single, post-run report
path. After
Process.run(), the resultingProcessExecutionReportownsnotification via
report.notify(*channels, only_errors=..., tasks=...), so thewhole run is delivered as one message instead of scattered per-task alerts.
Changes
channels are dropped. Tasks now only configure the traced-vars frame filter at
capture time, which feeds both the report and the per-task logfile.
report as process_name, used to label the email subject
(" | YYYYMMDD>") and emitted in the webhook payload alongside
entries.
slots=True on frozen value types.
README/docs for the report-only model.
Tests
notify_dispatch cover the new path.
matrix (2 languages × 4 palettes × 3 content styles × 2 only_errors = 48 emails).
Breaking changes
report.notify(channel, ...) after run().
EmailChannel/WebhookChannel); some previously public symbols are gone.
Type of change
feat— new featurefix— bug fixrefactor— no behavior changedocs— documentation onlytest— tests onlychore/ci/build