Feat: finetuningoptions validation telemetry - #6286
rsareddy0329 wants to merge 5 commits into
Conversation
Instrument FineTuningOptions.__setattr__ so client-side hyperparameter validation failures are captured in the SDK dev-experience logs, enabling detection and measurement of this class of failure: - setting an option name that is not a valid/overridable hyperparameter (raises AttributeError), and - setting a value outside its spec (type/range/enum; raises ValueError). Emission is failure-only: a single MODEL_CUSTOMIZATION FAILURE event is sent on the failure path, and nothing is emitted on successful sets, internal attribute sets, or construction, so no telemetry noise is added to the happy path. The emit is best-effort and never masks or replaces the user-facing validation error. Add unit tests covering both failure paths, the no-emit happy paths, and that a telemetry backend error still lets the validation error propagate.
|
Claude finished @rsareddy0329's task in 1m 24s —— View job Code Review: FineTuningOptions validation telemetry
The change is well-scoped and the mechanics are correct: the new imports ( 🔴 Telemetry opt-out is not honored on this new path
As written, a user who has explicitly opted out of telemetry will still have these validation-failure events emitted. That's a privacy/compliance regression and is inconsistent with the rest of the module. Please resolve the opt-out flag (using the resolved session = _get_default_sagemaker_session()
if not session:
return
if resolve_value_from_config(
direct_input=None,
config_path=TELEMETRY_OPT_OUT_PATH,
default_value=False,
sagemaker_session=session,
):
returnA unit test asserting 🟡
|
Move the failure-only telemetry emit out of FineTuningOptions and into the core telemetry module as a reusable _emit_failure_telemetry(feature, func_name, exc, sagemaker_session=None) helper. Unlike the @_telemetry_emitter decorator, which emits on success and failure for every call, this emits a single FAILURE event only when a caller explicitly hits a failure -- so it adds no happy-path telemetry or per-call overhead. It is best-effort and never raises. FineTuningOptions.__setattr__ now calls the shared helper on its two validation failure paths (invalid option name, out-of-spec value) instead of carrying its own emit plumbing, which shrinks the common.py change to a single import and two call sites. Add unit tests for the helper in the core telemetry suite and update the common.py tests to assert the helper is invoked on failures only.
rsareddy0329
changed the title
|
Claude finished @rsareddy0329's task in 58s —— View job Code Review: FineTuningOptions validation telemetry
The change is well-scoped and mechanically correct. The new 🔴 Telemetry opt-out is not honored on this new path
As written, a user who has explicitly opted out of telemetry will still have these validation-failure events emitted — a privacy/compliance regression and inconsistent with the rest of the module. Both session = sagemaker_session or _get_default_sagemaker_session()
if not session:
return
if resolve_value_from_config(
direct_input=None,
config_path=TELEMETRY_OPT_OUT_PATH,
default_value=False,
sagemaker_session=session,
):
returnA unit test asserting 🟡
|
|
nit: can you add details to PR description. |
Gate _emit_failure_telemetry on the TelemetryOptOut config flag (via resolve_value_from_config), matching the @_telemetry_emitter decorator, so a user who has opted out of telemetry does not have validation-failure events emitted. Also include the standard env/platform dimensions (x-env, x-sys, x-platform) in the payload for parity with decorator-emitted events, so the two can be sliced consistently. Add a unit test asserting no event is sent when opt-out is set.
|
Claude finished @rsareddy0329's task in 1m 41s —— View job Code Review: FineTuningOptions validation telemetry
This revision resolves the blocking issue from the earlier reviews. A few non-blocking observations: 🟡 Cross-field length validation raises but doesn't emit telemetry
🟡 Failure events omit the NOVA/OSS sub-feature dimension
Minor
Also, per @jam-jee's note: the PR description is currently empty — worth filling in a short summary of the change and its motivation. Overall: solid, well-tested, and the earlier blocking concern is addressed. The remaining items are optional. |
2 participants
Issue #, if available:
Description of changes:
Instruments FineTuningOptions.setattr so client-side hyperparameter validation failures are captured in the SDK dev-experience logs, enabling detection/measurement of this failure class:
Emission is failure-only via sagemaker.core.telemetry helper _emit_failure_telemetry: a single MODEL_CUSTOMIZATION FAILURE event is sent on the failure path, success path would not be captured as it records as multiple for one trainer call(per number of overridable params in recipe). The helper also adds the TelemetryOptOut config, mirrors the decorator's env/platform payload fields.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.