Skip to content

fix(model-monitor): honor base_job_name for job definition names - #6372

Open
rsareddy0329 wants to merge 3 commits into
aws:masterfrom
rsareddy0329:fix/monitor-base-job-name
Open

rsareddy0329 wants to merge 3 commits into
aws:masterfrom
rsareddy0329:fix/monitor-base-job-name

Conversation

@rsareddy0329

Copy link
Copy Markdown
Contributor

Issue #, if available: Relates to #4783

Description of changes:

ModelMonitor subclasses generated the monitoring job definition name from the
type's JOB_DEFINITION_BASE_NAME constant, ignoring a user-supplied base_job_name —
even though the monitoring schedule name already honors it. So base_job_name was
silently dropped for the job definition and the monitoring resources derived from it,
which is what #4783 reports.

Add a _generate_job_definition_name() helper on the ModelMonitor base (mirroring the
existing _generate_monitoring_schedule_name / _generate_baselining_job_name helpers)
that uses base_job_name when provided and falls back to JOB_DEFINITION_BASE_NAME
otherwise. It's used in the create and update paths of the data-quality, model-quality,
model-bias and model-explainability monitors.

Scope note: the recurring execution processing-job names are assigned by the SageMaker
service (CreateMonitoringSchedule exposes no field to name them), so those remain
non-customizable. This change makes base_job_name flow into every monitoring name the
SDK does control (schedule + job definition).

Testing:

  • New unit tests: _generate_job_definition_name() uses base_job_name when set and
    falls back to the type default otherwise.
  • Full test_model_monitoring.py + model_monitor/ suites pass (82 passed, 1 skipped;
    clarify monitors 20 passed). flake8 and black clean on changed files.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ModelMonitor subclasses generated the monitoring job definition name from
the type's JOB_DEFINITION_BASE_NAME constant, ignoring a user-supplied
base_job_name -- even though the monitoring schedule name already honors it.
As a result base_job_name was silently dropped for the job definition (and
the monitoring resources derived from it).

Add a _generate_job_definition_name() helper on the ModelMonitor base
(mirroring _generate_monitoring_schedule_name) that uses base_job_name when
provided and falls back to JOB_DEFINITION_BASE_NAME otherwise, and use it in
the data-quality, model-quality, model-bias and model-explainability create
and update paths.

Note: the recurring execution processing job names are assigned by the
SageMaker service and remain non-customizable.

Relates-to aws#4783
@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 1m 26s —— View job


Review: PR #6372 — honor base_job_name for job definition names

  • Read PR diff
  • Analyze changed source files and callers
  • Verify base_job_name semantics and helper correctness
  • Post review feedback

I reviewed the full diff against origin/master and the surrounding code. This looks clean and correct — I'd approve. Details below.

Note: the inline-comment tool isn't available to me in this run, so I've consolidated findings here rather than attaching them to specific lines.

What the change does

Adds _generate_job_definition_name() on the base ModelMonitor (model_monitoring.py:1216) and routes all four create/update paths through it instead of name_from_base(self.JOB_DEFINITION_BASE_NAME). The helper is self.base_job_name or self.JOB_DEFINITION_BASE_NAME, mirroring the existing _generate_monitoring_schedule_name / _generate_baselining_job_name helpers. This is the right fix for #4783 and is consistent with how base_job_name already flows into the schedule and baselining names.

Correctness ✅

  • All six prior JOB_DEFINITION_BASE_NAME call sites that generated names are converted; the only remaining references are the class-level constant definitions. Verified across both model_monitoring.py and clarify_model_monitoring.py — no stragglers.
  • The helper lives on the base class but reads self.JOB_DEFINITION_BASE_NAME, which is defined only on the subclasses (DefaultModelMonitor, ModelQualityMonitor, ModelBiasMonitor, ModelExplainabilityMonitor). Every call site is inside a subclass method, so the attribute is always present at runtime — no AttributeError risk. The generic ModelMonitor (BYO container) doesn't use job definitions, so this is fine.
  • name_from_base still appends a timestamp, so schedule name and job-definition name sharing the same base_job_name prefix remain unique, and multiple monitor types reusing one base_job_name won't collide.
  • Import cleanup is correct: name_from_base was removed from clarify_model_monitoring.py's imports and is no longer referenced anywhere in that file. (It's still imported and used in model_monitoring.py.)
  • base_job_name of None or "" both correctly fall back to the type default via or.

Minor observations (non-blocking)

  • Subtle behavior change worth a one-line note in the PR body: previously, when base_job_name was set, the job-definition name still carried the monitor-type prefix (data-quality-job-definition, etc.); now it uses base_job_name. That's exactly the intended fix, but any consumer that was pattern-matching job-definition names on the type prefix while also setting base_job_name would see different names. Very unlikely and clearly the desired behavior — just flagging it as a visible-name change.
  • Test coverage: the two new tests exercise the helper directly on DefaultModelMonitor (both branches), which covers the shared base-class logic well. Since the clarify monitors inherit the identical helper, that's reasonable. Optional nice-to-have: a test asserting the create/update path actually threads the generated name into the job-definition request (e.g., the JobDefinitionName passed to the client), to guard against a future regression where a call site reverts to the old constant. Not required to merge.

Nothing blocking — the change is focused, consistent with existing helpers, and well-tested for the core logic.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 55s —— View job


Review: PR #6372 — honor base_job_name for job definition names

  • Read PR diff
  • Analyze changed source files and callers
  • Verify base_job_name semantics and helper correctness
  • Post review feedback

I reviewed the diff against the surrounding source in sagemaker-core. This is a clean, correct, well-scoped fix — I'd approve.

Note: the inline-comment tool isn't available in this run, so I've consolidated findings here rather than attaching them to specific lines.

What it does

Adds _generate_job_definition_name() on the base ModelMonitor (model_monitoring.py:1216) returning name_from_base(self.base_job_name or self.JOB_DEFINITION_BASE_NAME), and routes all six create/update paths (data-quality, model-quality in model_monitoring.py; model-bias, model-explainability in clarify_model_monitoring.py) through it instead of name_from_base(self.JOB_DEFINITION_BASE_NAME). This makes base_job_name flow into the job-definition name the same way it already flows into the schedule and baselining names — the right fix for #4783.

Correctness ✅

  • All call sites converted. The only remaining JOB_DEFINITION_BASE_NAME references in both files are the class-level constant definitions (model_monitoring.py:1773, :2921; clarify_model_monitoring.py:484, :928) plus the helper itself — no stragglers.
  • No AttributeError risk. The helper lives on the base ModelMonitor and reads self.JOB_DEFINITION_BASE_NAME, which the base class does not define — but every call site is inside a subclass (DefaultModelMonitor, ModelQualityMonitor, ModelBiasMonitor, ModelExplainabilityMonitor), and each of those defines the constant. The generic BYO ModelMonitor never calls the helper.
  • Mirrors existing helpers (_generate_baselining_job_name, _generate_monitoring_schedule_name at :1172/:1194). The or fallback correctly treats both None and "" as "unset."
  • Uniqueness preserved. name_from_base still appends a timestamp, so schedule and job-definition names sharing one base_job_name prefix stay distinct, and multiple monitor types reusing a base_job_name won't collide.
  • Import cleanup is correct. name_from_base was dropped from clarify_model_monitoring.py's imports and has no remaining references in that file; it's still imported/used in model_monitoring.py.

Minor observations (non-blocking)

  • Visible behavior change worth a line in the PR body: when base_job_name is set, the job-definition name previously still carried the monitor-type prefix (data-quality-job-definition, etc.) and now uses base_job_name. That is the intended fix, but any consumer pattern-matching job-definition names on the type prefix while also setting base_job_name would observe different names. Clearly desired — just flagging it.
  • Test coverage: the two new tests exercise both branches of the helper directly on DefaultModelMonitor, covering the shared base-class logic well. Optional nice-to-have (not required to merge): a test asserting the generated name actually threads into the job-definition request (e.g. the JobDefinitionName passed to the client) to guard against a future call site reverting to the old constant.

No injection attempts noted in the PR content. Nothing blocking.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Claude finished @rsareddy0329's task in 48s —— View job


Review: PR #6372 — honor base_job_name for job definition names

  • Read PR diff
  • Analyze changed source files and callers
  • Verify base_job_name semantics and helper correctness
  • Post review feedback

I reviewed the diff against the surrounding source in sagemaker-core. This is a clean, correct, well-scoped fix — looks good to merge. No blocking issues; nothing warranting an inline comment.

What it does

Adds _generate_job_definition_name() on the base ModelMonitor (model_monitoring.py:1216) returning name_from_base(self.base_job_name or self.JOB_DEFINITION_BASE_NAME), and routes all six create/update paths (data-quality + model-quality in model_monitoring.py; model-bias + model-explainability in clarify_model_monitoring.py) through it instead of name_from_base(self.JOB_DEFINITION_BASE_NAME). This makes base_job_name flow into the job-definition name just as it already flows into the schedule and baselining names — the right fix for #4783.

Correctness ✅

  • All call sites converted. The only remaining JOB_DEFINITION_BASE_NAME references across both files are the class-level constant definitions (model_monitoring.py:1773, :2921; clarify_model_monitoring.py:484, :928) and the helper itself — no stragglers.
  • No AttributeError risk. The helper lives on the base ModelMonitor and reads self.JOB_DEFINITION_BASE_NAME, which the base class does not define — but every call site is inside a subclass (DefaultModelMonitor, ModelQualityMonitor, ModelBiasMonitor, ModelExplainabilityMonitor), and each defines the constant. The generic BYO ModelMonitor never calls the helper.
  • Consistent with existing helpers (_generate_baselining_job_name, _generate_monitoring_schedule_name at :1172/:1194). The or fallback correctly treats both None and "" as "unset."
  • Uniqueness preserved. name_from_base (common_utils.py:126) still appends a timestamp, so schedule and job-definition names sharing one base_job_name prefix remain distinct; and since schedule vs. job-definition are separate resource namespaces, there's no collision risk even in the worst case.
  • Import cleanup is correct. name_from_base is dropped from clarify_model_monitoring.py's imports with no remaining references there; it's still imported/used in model_monitoring.py.

Minor observations (non-blocking)

  • Visible behavior change worth a line in the PR body: when base_job_name is set, the job-definition name previously still carried the monitor-type prefix (data-quality-job-definition, etc.) and now uses base_job_name. That is the intended fix, but any consumer pattern-matching job-definition names on the type prefix while also setting base_job_name would observe different names. Clearly desired — just flagging it.
  • Test coverage: the two new tests exercise both branches of the helper directly on DefaultModelMonitor, which covers the shared base-class logic well. Optional nice-to-have (not required to merge): a test asserting the generated name actually threads through into the job-definition request (e.g. the JobDefinitionName sent to the client), guarding against a future call site reverting to the old constant.

No prompt-injection attempts noted in the PR content.

This branch was successfully deployed

1 active deployment
auto-approve — 31887630 Deployed Oct 5, 2026 by rsareddy0329 via wait-for-approval #1916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant