-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Feat]: Job Notifications for SMTJ #6042
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
Changes from all commits
793b59d
e281495
44c8930
8a82763
3c69274
e19096d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ | |
| ) | ||
| from sagemaker.train.common_utils.metrics_visualizer import plot_training_metrics | ||
| from sagemaker.train.common_utils.mlflow_config_utils import resolve_mlflow_tracking_fields | ||
| from sagemaker.train.common_utils.notifications import enable_notifications, delete_notification_rule, list_notification_rules | ||
| from sagemaker.train.common_utils.validator import validate_hyperpod_compute | ||
| from sagemaker.train.common_utils.cloudwatch_metrics import fetch_and_plot_metrics, _get_smhp_log_group | ||
| from sagemaker.train.defaults import TrainDefaults | ||
|
|
@@ -75,6 +76,12 @@ class BaseTrainer(ABC): | |
| training_image (Optional[str]): | ||
| Custom training container image URI. If not provided, the image is | ||
| auto-resolved from the model's recipe metadata in SageMaker Hub. | ||
| notifications (Optional[Dict[str, Any]]): | ||
| Configuration for SNS notifications on job status changes. Requires 'sns_topic_arn'. | ||
| Optional keys: 'events' ["Completed", "Failed", "Stopped"], 'event_bus_arn', | ||
| and 'job_name_prefix'. If not specified, no notifications are sent. | ||
| notification_rule_arn (str): | ||
| String of the EventBridge rule that is set up when enabling job notifications. | ||
| """ | ||
|
|
||
| # Class-level attributes with default values | ||
|
|
@@ -102,6 +109,7 @@ def __init__( | |
| training_image: Optional[str] = None, | ||
| base_model_name: Optional[str] = None, | ||
| disable_output_compression: Optional[bool] = False, | ||
| notifications: Optional[Dict[str, Any]] = None, | ||
| ): | ||
| self.sagemaker_session = sagemaker_session | ||
| self.role = role | ||
|
|
@@ -114,6 +122,11 @@ def __init__( | |
| self.training_image = training_image | ||
| self.base_model_name = base_model_name | ||
| self.disable_output_compression = disable_output_compression | ||
| self.notification_rule_arn = None | ||
|
|
||
| # Set up notifications if configured | ||
| if notifications: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we move setup notifs to def train(). Looks like the init is only meant for initializing some variables.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unless theres a good reason to have it here...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Additionally, looks like setup notifications returns an arn. We can log.debug that arn Also store it in self.notification_arn or something, in case user wants to retrieve it later.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not in I'll add the debug and saving the value for notifications.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense @ehsu3. |
||
| self.notification_rule_arn = self._setup_notifications(notifications) | ||
| self._checkpoint_s3_uri = None | ||
|
|
||
| def _is_nova_model_for_telemetry(self) -> bool: | ||
|
|
@@ -433,6 +446,98 @@ def _show_metrics_cloudwatch( | |
| end_time=end_time_ms, | ||
| ) | ||
|
|
||
| def _setup_notifications(self, notifications: Optional[Dict[str, Any]]) -> Optional[str]: | ||
| """Set up EventBridge notifications for the training job. | ||
|
|
||
| Called internally by trainer.train() after job submission when a | ||
| notifications config is provided. | ||
|
|
||
| Args: | ||
| notifications: Notification configuration dict with keys: | ||
| - sns_topic_arn (str, required): ARN of the SNS topic. | ||
| - events (list[str], optional): Job statuses to notify on. | ||
| Defaults to ["Completed", "Failed", "Stopped"]. | ||
| - event_bus_arn (str, optional): EventBridge bus ARN. | ||
| Defaults to the account's default bus. | ||
| - job_name_prefix (str, optional): Only notify for jobs | ||
| with names matching this prefix. | ||
|
|
||
| Returns: | ||
| The EventBridge rule ARN if notifications were set up, None otherwise. | ||
|
|
||
| Raises: | ||
| NotImplementedError: If compute is HyperPodCompute. | ||
| ValueError: If the config is invalid. | ||
| PermissionError: If the caller lacks required permissions. | ||
| """ | ||
| if not notifications: | ||
| return None | ||
|
|
||
| # Validate compute type | ||
| if isinstance(getattr(self, 'compute', None), HyperPodCompute): | ||
| raise NotImplementedError( | ||
| "Job notifications are not supported for HyperPod compute." | ||
| ) | ||
|
|
||
| # Validate config | ||
| if not isinstance(notifications, dict): | ||
| raise ValueError( | ||
| "notifications must be a dict with at least 'sns_topic_arn'. " | ||
| "Example: {'sns_topic_arn': 'arn:aws:sns:us-east-1:123456789012:my-topic'}" | ||
| ) | ||
|
|
||
| sns_topic_arn = notifications.get("sns_topic_arn") | ||
| if not sns_topic_arn: | ||
| raise ValueError( | ||
| "notifications config requires 'sns_topic_arn'. " | ||
| "Example: {'sns_topic_arn': 'arn:aws:sns:us-east-1:123456789012:my-topic'}" | ||
| ) | ||
|
|
||
| rule_arn = enable_notifications( | ||
| sns_topic_arn=sns_topic_arn, | ||
| sagemaker_session=TrainDefaults.get_sagemaker_session(sagemaker_session=self.sagemaker_session), | ||
| events=notifications.get("events"), | ||
| event_bus_arn=notifications.get("event_bus_arn"), | ||
| job_name_prefix=notifications.get("job_name_prefix"), | ||
| ) | ||
|
|
||
| logger.debug("Notification rule ARN: %s", rule_arn) | ||
| return rule_arn | ||
|
|
||
| def delete_notification_rule( | ||
| self, | ||
| rule_arn: str, | ||
| event_bus_arn: Optional[str] = None, | ||
| ) -> str: | ||
| """Delete an SDK-created EventBridge notification rule. | ||
|
|
||
| Args: | ||
| rule_arn: The ARN of the rule to delete. | ||
| event_bus_arn: Optional EventBridge bus ARN. Defaults to "default". | ||
|
|
||
| Returns: | ||
| The name of the deleted rule. | ||
| """ | ||
| return delete_notification_rule( | ||
| sagemaker_session=TrainDefaults.get_sagemaker_session(sagemaker_session=self.sagemaker_session), | ||
| rule_arn=rule_arn, | ||
| event_bus_arn=event_bus_arn, | ||
| ) | ||
|
|
||
| def list_notification_rules( | ||
| self, | ||
| event_bus_arn: Optional[str] = None, | ||
| ) -> List[Dict[str, str]]: | ||
| """List all SDK-created EventBridge notification rules. | ||
|
|
||
| Returns: | ||
| List of dicts with 'name', 'arn', and 'state' for each rule. | ||
| """ | ||
| return list_notification_rules( | ||
| sagemaker_session=TrainDefaults.get_sagemaker_session(sagemaker_session=self.sagemaker_session), | ||
| event_bus_arn=event_bus_arn, | ||
| ) | ||
|
|
||
| def stream_logs(self, poll: int = 5, start_time: Optional[Any] = None) -> None: | ||
| """Stream CloudWatch logs in real-time (like ``kubectl logs -f``). | ||
|
|
||
|
|
||
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.
Why not add notifications param in the trainer classes (sft, dpo etc) as well? This would make it easier to discover and use.
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.
I didn't think about adding that for visibility -- I can add that so it's easier to discover. I primarily just wanted to centralize the logic/implementation and reduce how much replication we have to do across the different trainers (hence base_trainer), but just adding the param isn't bad. I'll do that!
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.
Pros of adding to child trainers (SFT, DPO, etc.):
doesn't appear in SFTTrainer.init's signature, it won't show up in IDE
autocomplete or help().
to know the inheritance hierarchy.
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.
^^ Gen AI helped with the Pros