SMTP Email Alerter integration + make generic Alerter steps #3379
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 <noreply@anthropic.com>
strickvl
added
enhancement
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 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
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
strickvl
changed the title
- 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 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.SMTPconstructor 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_htmlfunction 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’smarkdownwith 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_htmlin 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.pysrc/zenml/hooks/alerter_hooks.pysrc/zenml/integrations/smtp_email/flavors/smtp_email_alerter_flavor.pysrc/zenml/integrations/smtp_email/steps/smtp_email_alerter_post_step.pysrc/zenml/integrations/smtp_email/__init__.pysrc/zenml/integrations/openai/hooks/open_ai_failure_hook.pysrc/zenml/integrations/smtp_email/hooks/email_alerter_hooks.pysrc/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.mddocs/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_classhelps 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 addingtracebackand 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
Theaskmethod 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.
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.
There was a problem hiding this comment.
I've heavily updated it based on this comment. Hope I haven't gone too far. There's no breaking changes, but it's def a fairly large change.
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.
- Extract 9 focused helper functions for each markdown transformation - Simplify main function to orchestrate transformations - Add comprehensive unit tests for each helper function (23 new tests) - Improve code readability and maintainability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Implement a unified approach for alerter hooks using the AlerterMessage model, providing better structured alerts across all alerter types (Slack, Discord, Email). - Update alerter_failure_hook and alerter_success_hook to use AlerterMessage with title, body, and metadata fields for better organization - Add graceful fallback to legacy string format for backwards compatibility - Add deprecation warnings in BaseAlerter.post() when strings are passed - Create comprehensive unit tests for both AlerterMessage and string formats - Update documentation to showcase AlerterMessage as the primary approach - Add hook usage examples to each alerter flavor documentation - Fix SMTP alerter docstring to resolve darglint validation errors This change improves consistency across alerter implementations while maintaining full backwards compatibility for existing users. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Never type was added to typing module in Python 3.11. For compatibility with earlier Python versions, import it from typing_extensions instead. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@BugBot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Resolved conflict in docs/book/component-guide/alerters/custom.md - Maintained AlerterMessage unified approach from email alerter feature - Integrated latest develop branch changes including documentation fixes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix mutable default arguments in AlerterMessage Pydantic model by using Field(default_factory) - Fix async cleanup failures on closed event loops in Discord alerter - Add proper RuntimeError handling for loop state checks - Ensure graceful cleanup without crashes when loops are already closed 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Create src/zenml/utils/markdown_utils.py with comprehensive markdown-to-HTML conversion - Support multiple output formats (HTML, Slack, Discord, Plain) with configurable options - Add preset configurations: EMAIL_MARKDOWN_OPTIONS, FULL_MARKDOWN_OPTIONS, BASIC_MARKDOWN_OPTIONS - Update SMTP Email Alerter to use unified utils with EMAIL_MARKDOWN_OPTIONS - Update OpenAI Failure Hook to use unified utils with FULL_MARKDOWN_OPTIONS - Eliminate ~245 lines of duplicate markdown processing code - Add comprehensive test suite with 46 test cases for unified utils - Remove obsolete test files and consolidate markdown testing - Maintain backward compatibility and enhance security with HTML escaping 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@BugBot run |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new SMTP Email Alerter integration and refactors existing Slack/Discord alerters to use a unified AlerterMessage model, plus adds generic alerter_post_step and alerter_ask_step steps.
- Added SMTP Email Alerter with HTML templates and proper traceback formatting
- Refactored Slack and Discord alerters to accept
AlerterMessageobjects - Introduced generic alerter steps and updated hooks for structured alerts
Reviewed Changes
Copilot reviewed 20 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/zenml/integrations/slack/steps/slack_alerter_ask_step.py | Deprecation warning for specialized ask step |
| src/zenml/integrations/slack/alerters/slack_alerter.py | Added AlerterMessage support for post/ask methods |
| src/zenml/integrations/openai/hooks/open_ai_failure_hook.py | Switched to Python traceback; added email‐specific formatting |
| src/zenml/integrations/discord/steps/discord_alerter_post_step.py | Deprecation warning for specialized post step |
| src/zenml/integrations/discord/steps/discord_alerter_ask_step.py | Deprecation warning for specialized ask step |
| src/zenml/integrations/discord/alerters/discord_alerter.py | Added AlerterMessage support; improved client cleanup |
| src/zenml/alerter/steps/alerter_post_step.py & alerter_ask_step.py | New generic alerter steps using AlerterMessage |
| src/zenml/alerter/base_alerter.py | Updated base methods to handle AlerterMessage |
| src/zenml/hooks/templates.py | New HTML templates for email success/failure |
| src/zenml/hooks/alerter_hooks.py | Hooks now emit structured AlerterMessage with fallback |
| src/zenml/integrations/constants.py | Added SMTP_EMAIL constant |
| docs/* | Updated documentation for generic steps and email alerter |
Comments suppressed due to low confidence (2)
src/zenml/integrations/openai/hooks/open_ai_failure_hook.py:220
- The
openai_chatgpt_alerter_failure_hooknow uses the model namegpt-4o, which doesn't align with the hook name. Please confirm the intended model and update either the hook name or the model identifier for consistency.
openai_alerter_failure_hook_helper(exception, "gpt-4o")
src/zenml/integrations/slack/steps/slack_alerter_ask_step.py:37
- [nitpick] Instead of placing the deprecation note in the docstring, consider using a standard deprecation decorator or directive (e.g.,
@deprecated) so IDEs and documentation generators can automatically flag this step as deprecated.
DEPRECATED: Please use `alerter_ask_step` instead. This step will be removed in a future release.
| # Add a message field | ||
| embed.add_field( | ||
| name=":email: *Message:*", value=f"\n{message}", inline=False | ||
| name=":email: *Message:*", |
There was a problem hiding this comment.
[nitpick] The embed field uses an email icon (:email:) for Discord messages, which may be confusing. Consider using a chat-specific icon (e.g., :speech_balloon:) to better match the platform.
| name=":email: *Message:*", | |
| name=":speech_balloon: *Message:*", |
| # Ensure client is closed properly | ||
| if not client.is_closed(): | ||
| if loop.is_running(): | ||
| # If loop is still running, schedule cleanup | ||
| asyncio.create_task(client.close()) | ||
| else: | ||
| # If loop is closed, we can't run cleanup | ||
| try: | ||
| loop.run_until_complete(client.close()) | ||
| except RuntimeError as e: | ||
| if "cannot be called on a running loop" not in str(e): | ||
| raise |
There was a problem hiding this comment.
[nitpick] The cleanup logic in start_client is quite complex with nested try/except blocks. Consider refactoring this into a dedicated helper function to improve readability and reduce nesting.
| # Ensure client is closed properly | |
| if not client.is_closed(): | |
| if loop.is_running(): | |
| # If loop is still running, schedule cleanup | |
| asyncio.create_task(client.close()) | |
| else: | |
| # If loop is closed, we can't run cleanup | |
| try: | |
| loop.run_until_complete(client.close()) | |
| except RuntimeError as e: | |
| if "cannot be called on a running loop" not in str(e): | |
| raise | |
| self._cleanup_event_loop(client, loop) |
| The configuration. | ||
| """ | ||
| return cast(BaseAlerterConfig, self._config) | ||
|
|
There was a problem hiding this comment.
The base post and ask methods currently return True by default. To ensure subclasses implement actual sending logic, consider making these methods abstract (e.g., using @abstractmethod) or raising NotImplementedError instead of a silent pass-through.
| @abstractmethod |
There was a problem hiding this comment.
Bug: Discord Alerter Shows "None" for Empty Messages
The Discord alerter incorrectly displays the literal string "None" when AlerterMessage.body is None. This occurs in the ask() method:
- When an
AlerterMessagewith aNonebody is passed to the_create_blockshelper function, the embed message field will display "None". - If no embed is used, the raw message content sent to Discord will also be "None".
src/zenml/integrations/discord/alerters/discord_alerter.py#L171-L178
zenml/src/zenml/integrations/discord/alerters/discord_alerter.py
Lines 171 to 178 in 8afcd0c
src/zenml/integrations/discord/alerters/discord_alerter.py#L368-L376
zenml/src/zenml/integrations/discord/alerters/discord_alerter.py
Lines 368 to 376 in 8afcd0c
Was this report helpful? Give feedback by reacting with 👍 or 👎
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
3 participants
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
AlerterMessagemodel for consistent alerter interfacesalerter_post_stepandalerter_ask_stepfor all alerter typesEnhanced Alerter Hooks (New)
alerter_failure_hookandalerter_success_hookto use structuredAlerterMessageformatalerter.post()Bug Fixes
default_subjectvariable in SMTP Email Alerter componentScreenshot
Tests run
Completed TODOs
Manual Testing with Example Pipelines
All three alerters have been successfully tested with example pipelines:
Discord Alerter: Pipeline Run
alerter_post_stepworkingalerter_ask_stepworkingSlack Alerter: Pipeline Run
alerter_post_stepworkingalerter_ask_stepworking (requires bot to be invited to channel)SMTP Email Alerter: Pipeline Run
Test code for all three alerters can be found in this Gist.