-
Notifications
You must be signed in to change notification settings - Fork 497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SMTP Email Alerter integration + make generic Alerter steps #3379
base: develop
Are you sure you want to change the base?
Conversation
This commit adds a new SMTP Email Alerter integration, allowing users to send email notifications from within ZenML pipelines and steps. Features: - Support for both plain text and HTML-formatted emails - Documentation with detailed examples and setup instructions - Support for customizable email subjects and recipients - Predefined step for easy integration into pipelines 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces SMTP email alerting capabilities into the ZenML framework. Documentation is updated to explain the new SMTP Email Alerter, its configuration, usage, and security advice. In the codebase, exception handling in failure hooks has been streamlined using standard traceback formatting and a markdown-to-HTML conversion helper was added for improved email message formatting. Additionally, a new constant for SMTP email integration is defined and several new components—including integrations, alerters, flavors, hooks, and post steps—were implemented to support the new email notification functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Step as Pipeline Step
participant Hook as SMTP Email Alerter Failure Hook
participant Alerter as SMTPEmailAlerter
participant SMTP as SMTP Server
Step->>Hook: Trigger failure event
Hook->>Alerter: Build email payload & parameters
Alerter->>SMTP: Connect (with TLS/auth) and send email
SMTP-->>Alerter: Acknowledgement of sent email
Alerter-->>Hook: Return success/failure status
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
src/zenml/integrations/smtp_email/alerters/smtp_email_alerter.py
Outdated
Show resolved
Hide resolved
- Replace warning log with a raised NotImplementedError - Update method docstring to reflect the new error handling approach - Clarify that interactive approvals are not supported for email alerters
- Create dedicated hooks for SMTP email alerters to provide better email formatting - Implement proper HTML formatting for both success and failure emails - Add markdown-style formatting support (backticks, asterisks, etc.) - Improve traceback display with monospaced formatting - Update documentation to explain the specialized hooks - Fix circular import issues in the integration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Use standard Python traceback instead of Rich for more concise error reporting - Update docs to include more details about email-specific hooks' benefits - Fix consistency in traceback handling throughout the code 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove all Rich traceback formatting code from hooks - Replace with Python's native traceback module for cleaner, more readable error messages - Update both standard and email-specific hooks to use the same approach - Remove unnecessary imports 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Clean pipeline and step names in email subjects to remove Markdown symbols - Ensure email subjects display cleanly without formatting artifacts - Fix issue where * or ` characters could appear in email subjects 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add proper type annotations for process_markdown_line function - Add List type annotation for formatted_parts - Clean up imports with explicit typing imports 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add detection of SMTP email alerter type - Replace Rich traceback with standard Python traceback - Create dedicated HTML email format with proper styling - Add comprehensive markdown-to-HTML converter for ChatGPT responses - Handle code blocks with language annotations - Convert numbered lists to bullet lists for better email display - Add proper type annotations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/zenml/integrations/smtp_email/steps/smtp_email_alerter_post_step.py (1)
26-71
: Well-structured email alerter step implementation.The step function is properly implemented with clear parameters, good error handling, and comprehensive documentation. It correctly integrates with the alerter system and provides flexibility for customizing email content.
I have a few suggestions for improvement:
Consider adding basic validation for the message parameter to handle empty strings:
@step def smtp_email_alerter_post_step( message: str, recipient_email: Optional[str] = None, subject: Optional[str] = None, include_pipeline_info: bool = True, ) -> None: """Step that sends an email alert using the active alerter. Args: message: Message to be included in the email. recipient_email: Optional recipient email address. If not provided, the email will be sent to the default recipient configured in the alerter. subject: Optional custom subject for the email. include_pipeline_info: If True, includes pipeline and step information in the email. Raises: RuntimeError: If no alerter is configured in the active stack. + ValueError: If message is empty. """ + if not message or not message.strip(): + raise ValueError("Message cannot be empty") + alerter = Client().active_stack.alerter if not alerter: raise RuntimeError( "No alerter configured in the active stack. " "Please configure an alerter in your stack." )Consider adding more helpful information in the docstring about how to format messages, particularly if the alerter supports HTML or Markdown formatting:
"""Step that sends an email alert using the active alerter. Args: message: Message to be included in the email. + HTML or Markdown formatting can be used for rich content. recipient_email: Optional recipient email address. If not provided, the email will be sent to the default recipient configured in the alerter. subject: Optional custom subject for the email. include_pipeline_info: If True, includes pipeline and step information in the email.
src/zenml/integrations/smtp_email/flavors/smtp_email_alerter_flavor.py (1)
62-80
: Consider handling possible timeouts and sensitive error handling inis_valid
.While this method is a solid approach for validating SMTP connectivity, you might consider:
- Setting a timeout on the
smtplib.SMTP
constructor in case of unresponsive servers.- Ensuring any sensitive exception details don’t leak into logs in production environments.
docs/book/component-guide/alerters/smtp_email.md (2)
186-186
: Add a comma after “alerter”.In the sentence, “These work with any alerter but may not provide optimal formatting for emails,” a comma after “alerter” can improve readability:
-These work with any alerter but may not provide optimal formatting for emails: +These work with any alerter, but may not provide optimal formatting for emails:🧰 Tools
🪛 LanguageTool
[uncategorized] ~186-~186: Possible missing comma found.
Context: ...erter Hooks These work with any alerter but may not provide optimal formatting for ...(AI_HYDRA_LEO_MISSING_COMMA)
236-242
: Use consistent list style to satisfy linting rules.Markdownlint is flagging the dash-based list style here. Switching from dashes to asterisks will comply with the MD004 rule:
- - Format messages with proper HTML for email clients - - Include relevant pipeline and step information - - Set descriptive email subjects - - Use structured layout for better readability - - Format error tracebacks for better legibility in email clients - - Support Markdown-style formatting + * Format messages with proper HTML for email clients + * Include relevant pipeline and step information + * Set descriptive email subjects + * Use structured layout for better readability + * Format error tracebacks for better legibility in email clients + * Support Markdown-style formatting🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
236-236: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
237-237: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
238-238: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
239-239: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
240-240: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
241-241: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
242-242: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
src/zenml/integrations/smtp_email/hooks/email_alerter_hooks.py (2)
28-77
: Avoid duplicated fallback handling logic in failure hook.While falling back to generic
alerter.post()
is useful if the alerter is not an SMTPEmailAlerter, consider extracting the fallback logic into a shared helper method to reduce duplication and keep the hook more concise.
175-288
: Refactor HTML templates for better maintainability.The success hook’s HTML generation mirrors much of the logic used in the failure hook, differing mainly by displayed content. You might centralize or parametricize the HTML template to reduce code duplication and improve long-term maintainability.
src/zenml/integrations/openai/hooks/open_ai_failure_hook.py (4)
28-186
: Consider using a standard Markdown library
The custom_format_markdown_for_html
function uses regex-based transformations and manually escapes HTML. While this works, it can be error-prone for more complex Markdown features and poses potential XSS risks if any untrusted input slips through. A well-tested library, such as Python’smarkdown
with controlled extensions, could simplify maintenance and reduce vulnerabilities.
254-255
: Ensure sensitive info is not exposed in prompts
Using{exception}
and the full traceback in the prompt to the OpenAI model might unintentionally expose private data or credentials. Consider redacting sensitive information before sending it to external services.Would you like me to propose a secure redaction process for these fields?
273-273
: Validate GPT suggestions for safety
Although the suggestion is appended as plain text here, confirm that any HTML or formatting from GPT is sanitized before use or display, especially if rendering beyond plain text contexts.
277-379
: Separate SMTP email-building logic for clarity
Building the message, subject, and HTML body inline makes the code lengthy and less modular. Extracting HTML creation and subject assembly into helper methods could improve maintainability and readability.src/zenml/integrations/smtp_email/alerters/smtp_email_alerter.py (2)
181-314
: Avoid duplicating markdown/HTML parsing logic
Certain markdown processing here overlaps with_format_markdown_for_html
in the OpenAI hooks. Consider consolidating or reusing a shared helper function to reduce code duplication and potential drift.
315-378
: Use a context manager for SMTP
Inpost
, manually callingserver.quit()
works but a context manager (e.g.,with smtplib.SMTP(...) as server:
) is often safer and ensures proper cleanup even upon exceptions. Additionally, consider capturing and re-raising critical SMTP errors to alert the system if the email could not be sent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/book/component-guide/alerters/alerters.md
(1 hunks)docs/book/component-guide/alerters/smtp_email.md
(1 hunks)src/zenml/hooks/alerter_hooks.py
(2 hunks)src/zenml/integrations/constants.py
(1 hunks)src/zenml/integrations/openai/hooks/open_ai_failure_hook.py
(7 hunks)src/zenml/integrations/smtp_email/__init__.py
(1 hunks)src/zenml/integrations/smtp_email/alerters/__init__.py
(1 hunks)src/zenml/integrations/smtp_email/alerters/smtp_email_alerter.py
(1 hunks)src/zenml/integrations/smtp_email/flavors/__init__.py
(1 hunks)src/zenml/integrations/smtp_email/flavors/smtp_email_alerter_flavor.py
(1 hunks)src/zenml/integrations/smtp_email/hooks/__init__.py
(1 hunks)src/zenml/integrations/smtp_email/hooks/email_alerter_hooks.py
(1 hunks)src/zenml/integrations/smtp_email/steps/__init__.py
(1 hunks)src/zenml/integrations/smtp_email/steps/smtp_email_alerter_post_step.py
(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/zenml/integrations/smtp_email/steps/init.py
- src/zenml/integrations/smtp_email/alerters/init.py
- src/zenml/integrations/smtp_email/flavors/init.py
- src/zenml/integrations/smtp_email/hooks/init.py
🧰 Additional context used
📓 Path-based instructions (2)
`src/zenml/**/*.py`: Review the Python code for conformity w...
src/zenml/**/*.py
: Review the Python code for conformity with Python best practices.
src/zenml/integrations/constants.py
src/zenml/hooks/alerter_hooks.py
src/zenml/integrations/smtp_email/flavors/smtp_email_alerter_flavor.py
src/zenml/integrations/smtp_email/steps/smtp_email_alerter_post_step.py
src/zenml/integrations/smtp_email/__init__.py
src/zenml/integrations/openai/hooks/open_ai_failure_hook.py
src/zenml/integrations/smtp_email/hooks/email_alerter_hooks.py
src/zenml/integrations/smtp_email/alerters/smtp_email_alerter.py
`docs/**/*.md`: Review the documentation for readability and...
docs/**/*.md
: Review the documentation for readability and clarity.
docs/book/component-guide/alerters/alerters.md
docs/book/component-guide/alerters/smtp_email.md
🪛 LanguageTool
docs/book/component-guide/alerters/smtp_email.md
[uncategorized] ~23-~23: Possible missing comma found.
Context: ..., no additional integrations need to be installed as it uses the standard library's `smtp...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...at the parameters mean: * smtp_server
: Your email provider's SMTP server (e.g....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~186-~186: Possible missing comma found.
Context: ...erter Hooks These work with any alerter but may not provide optimal formatting for ...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/book/component-guide/alerters/smtp_email.md
236-236: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
237-237: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
238-238: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
239-239: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
240-240: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
241-241: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
242-242: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (15)
src/zenml/integrations/constants.py (1)
70-70
: New SMTP Email integration constant added.This constant will be used to identify the new SMTP Email Alerter integration throughout the codebase.
docs/book/component-guide/alerters/alerters.md (4)
2-2
: Documentation updated to include email as a notification method.The description now correctly reflects the addition of email alerts alongside chat services.
8-10
: Description expanded to include email notifications.The introduction now clearly explains that Alerters can send messages to both chat services and email, improving user understanding of the component's capabilities.
14-14
: SMTP Email Alerter added to available integrations list.The text now properly references the new alerter and its documentation page.
16-21
: Table updated to include SMTP Email Alerter.The table now includes the new SMTP Email alerter with appropriate flavor and integration information. The formatting and structure of the table are maintained consistently.
src/zenml/hooks/alerter_hooks.py (2)
16-16
: Improved exception handling for better email compatibility.The switch from Rich console to standard Python traceback formatting is a good improvement for email compatibility. This change ensures that exception information will display properly in email clients.
Also applies to: 36-41
48-51
: Enhanced exception message formatting.The updated format for exception messages now includes the exception type name along with the exception message, providing more valuable information to users receiving the alerts.
src/zenml/integrations/smtp_email/flavors/smtp_email_alerter_flavor.py (1)
82-140
: Well-structured flavor class.All the property methods (e.g.,
name
,docs_url
,sdk_docs_url
,config_class
,implementation_class
) cleanly encapsulate flavor details. The usage of a dedicatedconfig_class
helps maintain clarity and consistency.src/zenml/integrations/smtp_email/__init__.py (1)
48-66
: Cleanly handling potential circular imports.Your approach to registering the integration first, then conditionally importing hooks, is well designed to avoid import cycles. This structure is both practical and transparent.
src/zenml/integrations/openai/hooks/open_ai_failure_hook.py (3)
16-17
: Imports look appropriate
No issues identified with addingtraceback
and typing (List
,Match
,Optional
).
234-237
: Redact or sanitize traceback details
Forwarding complete tracebacks via email may leak secrets or sensitive information. Consider sanitizing or redacting certain data, especially in production.
397-398
: Verify custom model availability
Models “gpt-4o” and “gpt-4” are referenced here. Make sure “gpt-4o” is valid and accessible via the OpenAI API.Also applies to: 408-409
src/zenml/integrations/smtp_email/alerters/smtp_email_alerter.py (3)
16-19
: Imports are valid
No issues spotted with the SMTP and MIME-related imports; these are standard for sending emails in Python.
82-129
: Recipient email retrieval is well-structured
The logic for retrieving and validating the recipient email from parameters, settings, or the config is clear. Raising a ValueError if not found is appropriate.
379-398
: Interactive approvals not supported
Theask
method correctly raisesNotImplementedError
, reflecting that email-based alerters cannot handle interactive approvals. This is consistent with the alerter’s design.
src/zenml/integrations/smtp_email/alerters/smtp_email_alerter.py
Outdated
Show resolved
Hide resolved
RuntimeError: If no alerter is configured in the active stack. | ||
""" | ||
alerter = Client().active_stack.alerter | ||
if not alerter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also check the alerter type here?
In general it feels kinda weird to me what is happening with alerters. Shouldn't there be one alerter post step, which works independently of the alerter type? This is the whole reason for stack switching.
I would suggest that we think of a generic format that get's passed to alerter.post(...)
, and we can then get rid of all these specific steps. We can also avoid checks in the open AI hooks for example that handle the email alerter separately. The alerter itself should be responsible for the formatting, not some outside logic IMO.
src/zenml/integrations/smtp_email/alerters/smtp_email_alerter.py
Outdated
Show resolved
Hide resolved
…necessary installation check This commit simplifies the `SMTPEmailIntegration` class by removing the installation check and the circular import handling for hooks. The hooks are now expected to be imported directly from the hooks module in user code, streamlining the integration process.
This commit modifies the type hint of the `ask` method in the `SMTPEmailAlerter` class from `bool` to `Never`, indicating that this method is not intended to return a value. This change enhances code clarity and aligns with the method's purpose of not supporting interactive approvals for email alerters.
This commit simplifies the recipient email validation logic in the `SMTPEmailAlerter` class by removing unnecessary type checks. The condition now directly checks if `params` is truthy and if `recipient_email` is not `None`, enhancing code readability and maintainability.
Create a central _get_attribute_value method to consolidate duplicate code pattern for configuration attribute resolution. This method handles the shared logic of checking parameters, settings, and config values with consistent error handling. Refactored _get_recipient_email, _should_include_html, and _get_subject_prefix to use the new generic method while maintaining backward compatibility. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Create a new templates.py module in the hooks package with reusable email templates - Implement a flexible template system with customizable sections and styling - Refactor both SMTP Email and OpenAI alerter hooks to use the shared templates - Update documentation to reflect the new template system - Fix a typo in the OpenAI import statement in the hooks documentation This refactoring reduces code duplication, improves maintainability, and ensures consistent styling across different alerter integrations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add `Any` import from typing module - Fix type annotation for param_class in _get_attribute_value method - Change return type from `any` to `Any` in method signature - Add proper type casting with `cast()` for methods returning specific types 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Update documentation for all alerter flavors (Slack, Discord, SMTP Email, Custom) - Add details about the new AlerterMessage model - Document the generic alerter_post_step and alerter_ask_step - Update code examples to show the new unified approach - Update custom alerter implementation guidance 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Update DiscordAlerter to handle AlerterMessage objects - Add logic to format AlerterMessage content for Discord - Add deprecation warnings to discord alerter steps - Improve type hints and docstrings 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Update SlackAlerter to handle AlerterMessage objects - Add formatting logic for AlerterMessage content in Slack - Add deprecation warnings to slack_alerter_post_step and slack_alerter_ask_step - Improve type hints and error handling 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Update SMTPEmailAlerter to handle AlerterMessage objects - Add smart email formatting from AlerterMessage (title as subject, body as content) - Add deprecation warnings to smtp_email_alerter_post_step - Improve HTML formatting and content extraction from AlerterMessage - Enhance type hints and documentation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add new AlerterMessage model for structured alert content - Create generic alerter_post_step and alerter_ask_step - Update BaseAlerter to support both string and AlerterMessage objects - Add support for title, body, metadata, and images in messages - Standardize alerter interface across all flavors This change unifies the alerter steps across all flavors, allowing users to use a single consistent interface regardless of which alerter is configured in their stack. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Move AlerterMessage from alerter/message_models.py to models/v2/misc/alerter_models.py - Update all import references in code and documentation - Follow ZenML's architectural pattern of placing models in the models directory - This makes the model more discoverable and follows the project's structural conventions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add type_ignore for discord module imports - Fix AlerterMessage vs. string type handling in both BaseAlerter and implementations - Update BaseAlerter.ask() to accept either string or AlerterMessage - Fix typed event decorators in Discord alerter - Add proper string conversion in alerter subject line handling - Fix AlerterMessage body and title access to handle None values 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Added missing license headers to alerter_ask_step.py and alerter_post_step.py - Added missing module-level docstrings to fix ruff D100 errors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
@strickvl conflicts need a fixing :-) Just soft ping |
The PR adds a new SMTP Email Alerter integration to ZenML, enabling users to send email notifications on pipeline steps events. It also implements a unified alerter approach with a standardized AlerterMessage model across all alerter types.
Key Features
New SMTP Email Alerter Integration
Unified Alerter Architecture
AlerterMessage
model for consistent alerter interfacesalerter_post_step
andalerter_ask_step
for all alerter typesBug Fixes
default_subject
variable in SMTP Email Alerter componentScreenshot
Tests run
Completed TODOs