fix(train): raise on expired credentials in show_metrics log fetch - #6114
Open
skamal23 wants to merge 2 commits into
Open
fix(train): raise on expired credentials in show_metrics log fetch#6114skamal23 wants to merge 2 commits into
skamal23 wants to merge 2 commits into
Conversation
| {"message": "global_step: 2 train_rm_score: 0.72"}, | ||
| ] | ||
|
|
||
| AUTH_ERROR_CODES = [ |
Contributor
There was a problem hiding this comment.
Can we have a common constants in sagemaker-train/src/sagemaker/train/common_utils/constants.py so that they're reusable in both test and src files ? Otherwise, we need to maintain two lists for test coverage
Author
There was a problem hiding this comment.
Good call, moved it. Also rebased onto latest master-nova-follow-ups
amazeAmazing
suggested changes
Jul 28, 2026
added 2 commits
July 28, 2026 18:26
When AWS credentials expire, show_metrics() reported "No CloudWatch logs found for job '<name>'. The job may still be starting, or logs may not be available yet", sending users to debug their training job instead of refreshing credentials. _fetch_smtj_logs() and _fetch_smhp_logs() each wrapped their CloudWatch Logs call in a bare `except Exception`, logged a warning, and returned an empty list, so an ExpiredTokenException was indistinguishable from a job that genuinely has no logs yet. The empty list then became the misleading ValueError above. Design follows the existing notifications.py pattern: read the structured error code, re-raise the auth subset as PermissionError with remediation guidance, and let every other code keep its current degrade-to-empty behavior. A genuinely absent log group (ResourceNotFoundException) still returns an empty list, so a just-started job continues to raise the existing "No CloudWatch logs found" ValueError. Changes: - cloudwatch_metrics.py: add _AUTH_ERROR_CODES and _raise_if_auth_error(); apply at describe_log_streams, get_log_events, and filter_log_events; narrow `except Exception` to `except ClientError` - base_trainer.py: document PermissionError on show_metrics() - test_cloudwatch_metrics.py: 20 unit tests covering all 7 auth codes on both platforms, ResourceNotFoundException degradation, mid-pagination failure, and a regression guard for the existing "no logs found" path
Addresses review feedback: the auth error-code list was duplicated in cloudwatch_metrics.py and its unit test, so the two lists had to be kept in sync manually. Move it to the existing common_utils/constants.py as a module-level frozenset and import it in both places. Test parametrization sorts the frozenset so test IDs stay deterministic.
skamal23
force-pushed
the
fix/show-metrics-expired-credentials
branch
from
July 28, 2026 18:35
ba5c09c to
6e2b58e
Compare
amazeAmazing
approved these changes
Jul 28, 2026
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
2 participants
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.
When AWS credentials expire, show_metrics() reported "No CloudWatch logs found for job ''. The job may still be starting, or logs may not be available yet", sending users to debug their training job instead of refreshing credentials.
_fetch_smtj_logs() and _fetch_smhp_logs() each wrapped their CloudWatch Logs call in a bare
except Exception, logged a warning, and returned an empty list, so an ExpiredTokenException was indistinguishable from a job that genuinely has no logs yet. The empty list then became the misleading ValueError above.Changes:
except Exceptiontoexcept ClientError