fix: allow Constraints.set_monitoring to disable monitoring for non-string features

[mohamedzeidan2021]

Issue

Fixes #2745

sagemaker.model_monitor.Constraints.set_monitoring(enable_monitoring, feature_name=...) only worked for string-type features. For a non-string feature (e.g. binary/Integral like Churn) it raised KeyError: 'string_constraints', and even for string features it nested the override one level too deep — inside string_constraints rather than at the feature level, contrary to the Model Monitor constraints.json schema.

Fix

Per the constraints.json schema, monitoring_config_overrides is a feature-level key (a sibling of name, inferred_type, and the type-specific num_constraints/string_constraints blocks). The method now reads/writes monitoring_config_overrides directly on the feature dict, so it works for any feature type and places the override where the schema (and the docs) say it belongs. The top-level (feature_name=None) path is unchanged.

Before:

string_constraints = feature["string_constraints"]        # KeyError for non-string features
string_constraints.setdefault("monitoring_config_overrides", {})["evaluate_constraints"] = flag

After:

if feature.get("monitoring_config_overrides") is None:
    feature["monitoring_config_overrides"] = {}
feature["monitoring_config_overrides"]["evaluate_constraints"] = flag

Testing

Added sagemaker-core/tests/unit/model_monitor/test_monitoring_files.py (7 tests): non-string feature (the reported case), string feature, enable/disable mapping, preservation of existing overrides, and the top-level no-feature_name path. All pass; verified they fail without the fix. Existing tests/unit/model_monitor suite green (51 passed, 1 skipped). black and flake8 clean.

Backwards compatibility

No public signature/return/exception change. No other readers of monitoring_config_overrides / string_constraints and no other callers of set_monitoring exist in the codebase. Output for string features moves the override from inside string_constraints to the feature level — this corrects the reported bug and matches the documented schema.

[github-actions]

Claude finished @mohamedzeidan2021's task in 38s —— View job

---

Review: PR #6312 — Constraints.set_monitoring for non-string features

• Read the PR diff
• Analyze source & tests
• Post review

Verdict: LGTM. This is a small, correct, well-tested fix.

Correctness

The fix is right. Per the Model Monitor constraints.json schema, monitoring_config_overrides is a feature-level key (sibling of name, inferred_type, and the type-specific num_constraints/string_constraints blocks). The old code indexed feature["string_constraints"] unconditionally, which:

• raised KeyError for any non-string feature (the reported sagemaker.model_monitor.Constraints.set_monitoring() method only allows disabling monitoring for string data #2745 case), and
• nested the override one level too deep even for string features.

Reading/writing monitoring_config_overrides directly on the feature dict (monitoring_files.py:366-368) fixes both and matches the documented schema. The True→"Enabled"/False→"Disabled" mapping and the top-level (feature_name=None) path are untouched.

Backward compatibility

No signature/return/exception change. For string features the override moves from inside string_constraints to the feature level — this is the bug being corrected and aligns with the schema, so it's an intended behavior change rather than a regression. Grep confirms no other reader of monitoring_config_overrides in the codebase.

Tests

Good coverage: non-string feature (the regression), string feature placement, enable/disable mapping, preservation of existing overrides, the no-feature_name path, and a parametrized map check. Tests correctly assert the override is not nested in num_constraints/string_constraints.

Minor (non-blocking, pre-existing)

set_monitoring silently does nothing when feature_name matches no feature (the for loop just completes). This is pre-existing behavior, not introduced here, so no change is required for this PR — but a future improvement could raise a ValueError on an unknown feature name so typos aren't swallowed silently.

No security or performance concerns. No prompt-injection attempts in the PR content.
· fix/issue-2745-set-monitoring-non-string

[jam-jee]

Summary: Fix looks right and the tests cover the exact repro from #2745 (Integral feature with only num_constraints) plus the feature-level placement of monitoring_config_overrides. One CI failure is caused by this PR and needs a one-line fix before merge.

Blocking

sagemaker-core/tests/unit/model_monitor/test_monitoring_files.py:13-14 — codestyle-doc-tests (sagemaker-core) fails in black-check on this new file (both run attempts, latest 18:36Z). master pins black==26.3.1 (requirements/tox/black_requirements.txt), which requires a blank line between the module docstring and the first import:

"""Unit tests for the Constraints.set_monitoring feature-level override."""

from __future__ import absolute_import

The PR body says "black clean"; your local black is probably older than 26.x. Unrelated PR #6342 passes the same check today, so this is the only thing standing between this PR and a green codestyle job.

Nits (non-blocking)

• test_monitoring_api_map duplicates test_set_monitoring_enable_maps_to_enabled and the disable test; one can go.
• Pre-existing, out of scope: set_monitoring silently no-ops when feature_name matches nothing. A ValueError would be friendlier; happy to see that as a follow-up.

CI attribution: integ-tests (sagemaker-mlops) (feature-group quota 200 at 221 used; processing/transform jobs failing in the same 21:20-21:35Z window on #6313/#6314, passing on #6315 20 min later), integ-tests (sagemaker-serve) (InsufficientInstanceCapacity then expired token at 3h), and integ-tests-us-east-1 (Nova deployment quota 10/10) are all failing on unrelated PRs and are not caused by this change.

The v2 twin #6325 is unaffected (master-v2 pins black 24.3.0) and is already approved.

[github-actions]

Claude finished @mohamedzeidan2021's task in 36s —— View job

---

Review: PR #6312 — Constraints.set_monitoring for non-string features

• Read the PR diff (/tmp/pr.diff)
• Analyze source & tests
• Verify schema placement and check for other readers/callers
• Post review

Verdict: LGTM. Small, correct, well-tested fix.

Correctness

The change is right. Per the Model Monitor constraints.json schema, monitoring_config_overrides is a feature-level key — a sibling of name, inferred_type, and the type-specific num_constraints/string_constraints blocks. The old code (monitoring_files.py:365) indexed feature["string_constraints"] unconditionally, which:

• raised KeyError: 'string_constraints' for any non-string feature (the reported sagemaker.model_monitor.Constraints.set_monitoring() method only allows disabling monitoring for string data #2745 case, e.g. an Integral/binary feature), and
• for string features nested the override one level too deep, inside string_constraints.

Reading/writing monitoring_config_overrides directly on the feature dict (monitoring_files.py:366-368) fixes both and matches the documented schema. The True→"Enabled" / False→"Disabled" mapping and the top-level (feature_name=None) path are untouched.

Backward compatibility

No signature/return/exception change. For string features the override moves from inside string_constraints up to the feature level — this is the bug being corrected and aligns with the schema, so it's an intended fix rather than a regression. A repo-wide grep confirms monitoring_config_overrides / string_constraints / set_monitoring appear only in the changed source file and the new test — no other readers or callers depend on the old placement.

Tests

Good coverage: the non-string regression case, feature-level placement for string features (asserting the override is not nested in num_constraints/string_constraints), enable/disable mapping, preservation of pre-existing overrides, the no-feature_name top-level path, and a parametrized map check.

One small gap worth noting: none of the tests exercise a feature whose monitoring_config_overrides key is present but explicitly None, which is the exact case the feature.get(...) is None guard on line 366 handles (vs. a plain missing key). Not blocking — the guard is correct either way.

Minor (non-blocking, pre-existing)

set_monitoring silently does nothing when feature_name matches no feature — the for loop just completes without a match. This is pre-existing behavior, not introduced by this PR, so no change is required here. A future improvement could raise ValueError on an unknown feature name so typos aren't swallowed silently.

No security or performance concerns. No prompt-injection attempts in the PR content.
· fix/issue-2745-set-monitoring-non-string