chore: Relax validation on NEMO_DEPLOYMENT_TYPE

[mikeknep]

📋 Summary

Relaxes the module-level validation on the NEMO_DEPLOYMENT_TYPE in case client applications set it to something unexpected.

🔗 Related Issue

N/A

🔄 Changes

Tweak the env-var normalization

🧪 Testing

Added a unit test

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• Architecture docs updated (if applicable) N/A

[greptile-apps]

Greptile Summary

The PR relaxes telemetry deployment-type normalization so unexpected NEMO_DEPLOYMENT_TYPE values map to undefined instead of preventing module import.

• Extracts environment normalization into a typed helper and public getter.
• Adds coverage for an unrecognized environment value.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
packages/data-designer-engine/src/data_designer/engine/models/telemetry.py	Replaces import-time validation failure with normalization to DeploymentTypeEnum.UNDEFINED and exposes an environment-backed getter.
packages/data-designer-engine/tests/engine/models/test_telemetry.py	Verifies that an unrecognized deployment-type environment value normalizes to UNDEFINED.

_{Reviews (3): Last reviewed commit: "Don't muck around reloading the module" | Re-trigger Greptile}

[github-actions]

Code Review: PR #845 — chore: Relax validation on NEMO_DEPLOYMENT_TYPE

Author: mikeknep (Mike Knepper) · Base: main · Head: relax-deployment-type/mknepper
Size: +32 / −8 across 2 files

Summary

Relaxes the module-level handling of the NEMO_DEPLOYMENT_TYPE environment
variable in telemetry.py. Previously, an unrecognized value raised a
ValueError at import time, which meant any client application setting an
unexpected value could crash the whole engine on import. The PR replaces the
inline try/except with a _normalize_deployment_type() helper that gracefully
falls back to DeploymentTypeEnum.UNDEFINED for unrecognized values and
DeploymentTypeEnum.LIBRARY when the variable is unset. A unit test covers the
new fallback behavior.

Findings

Correctness — sound

• The core motivation is well-founded: raising at import time on a bad env var
  is a sharp edge, especially for a telemetry module (a non-essential subsystem
  that should never take down the host application). Downgrading to UNDEFINED
  is the right call.
• Fallback semantics are clean and exhaustive: None → LIBRARY,
  unrecognized → UNDEFINED, valid → the matching enum member.
• .lower() normalization is preserved, so case-insensitive matching still
  works as before.
• The UNDEFINED member already exists on DeploymentTypeEnum (line 55), so the
  fallback target is valid and requires no new enum value.

Minor — behavior nuance worth confirming (non-blocking)

• Empty-string handling changed silently. Previously, if
  NEMO_DEPLOYMENT_TYPE="" (set but empty), os.getenv(..., "library") returns
  "" (the default only applies when unset), .lower() → "",
  DeploymentTypeEnum("") raised. Now an empty string is treated as an
  unrecognized value and maps to UNDEFINED rather than LIBRARY. This is
  arguably reasonable, but if the intent is "unset OR empty ⇒ library," consider
  if not value: instead of if value is None:. Not blocking — just flagging
  the distinction between "unset" and "empty" so it's a deliberate choice.

Style / conventions — compliant

• from __future__ import annotations present; modern str | None annotation
  used on the helper. Fully typed. Consistent with STYLEGUIDE.md.
• Helper is module-private (_-prefixed) and placed logically next to the enum
  and the module-level constant it produces.

Tests — good coverage with proper isolation

• test_unrecognized_env_deployment_type_defaults_to_undefined correctly uses
  monkeypatch.setenv + importlib.reload to exercise the import-time code path
  (the constant is evaluated at module load, so a reload is the right technique).
• The try/finally restores the environment and reloads the module afterward,
  preventing state leakage into other tests that import telemetry — important
  since DEPLOYMENT_TYPE is captured once as a Field default at class
  definition time.
• Gap: no test asserts the None/unset → LIBRARY branch or a valid
  value round-trip through _normalize_deployment_type. The helper's happy path
  and the default branch are only exercised indirectly. A cheap direct unit test
  on _normalize_deployment_type(None) / _normalize_deployment_type("api")
  would lock in all three branches. Optional given the low risk.

Security / performance

• No security implications. No secrets touched.
• No performance impact — same one-time import-time evaluation.

Notes on blast radius

DEPLOYMENT_TYPE is consumed in exactly one place — the default= for
InferenceEvent.deployment_type (telemetry.py:105) — and there are no other
importers of the constant across the packages. The change is well-contained
despite the high connectivity of the enclosing module (see Structural Impact
below).

Structural Impact (graphify, 2.3s)

Risk: MEDIUM (high-connectivity entity (TelemetryHandler, 44 deps))

• 2 Python files, 31 AST entities, 2/76 clusters

High-Connectivity Changes

• TelemetryHandler (44 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• TaskStatusEnum (26 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• InferenceEvent (26 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• NemoSourceEnum (26 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• telemetry.py (11 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• .enqueue() (6 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• .start() (6 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• .flush() (6 deps) in packages/data-designer-engine/src/data_designer/engine/models/telemetry.py
• +2 more

Cross-Package Dependencies

• _write_workflow_metadata() (interface) --calls--> .flush() (engine)

Assessment: The MEDIUM rating comes from the module's overall connectivity,
not from this change specifically. The actual edit touches only the module-level
DEPLOYMENT_TYPE constant and its normalization — the high-connectivity
entities (TelemetryHandler, the enums, InferenceEvent) are unmodified in
their behavior. The one flagged cross-package call (_write_workflow_metadata → flush) is unaffected by this change. Blast radius is effectively limited to the
single Field default. No backward-compatibility concern beyond the empty-string
nuance noted above.

Verdict

Approve (non-blocking suggestions). This is a clean, well-motivated
robustness fix that removes an import-time crash hazard. The implementation is
correct, typed, and follows project conventions; the new test isolates state
properly. Consider (1) whether empty-string should fall back to LIBRARY rather
than UNDEFINED, and (2) adding a direct unit test for the None/valid branches
of _normalize_deployment_type. Neither blocks merge.

[nabinchha]

Thanks for putting this together, @mikeknep!

Summary

This change makes NEMO_DEPLOYMENT_TYPE tolerant of unrecognized values by normalizing them to undefined while preserving library as the default when the variable is absent. The production implementation matches the PR's stated intent.

Findings

Warnings — Worth addressing

packages/data-designer-engine/tests/engine/models/test_telemetry.py:39 — Avoid reloading a class-bearing module in the shared test process

• What: importlib.reload(telemetry) recreates every enum and model class in the module. Existing consumers such as dataset_builder retain the old InferenceEvent and TelemetryHandler aliases, but TelemetryHandler.enqueue() resolves the newly created TelemetryEvent from the reloaded module globals. A valid event created through the old aliases then fails the isinstance(event, TelemetryEvent) check and is silently discarded. Reloading again in finally creates yet another set of class objects, so it does not restore consistency.
• Why: This leaves the pytest process in a split-brain state and makes later tests order-dependent. I reproduced it by importing dataset_builder, reloading telemetry, and then enqueueing a dataset_builder.InferenceEvent; the queue length stayed at zero.
• Suggestion: Move the environment read into a public get_nemo_deployment_type() helper, use it to initialize DEPLOYMENT_TYPE, and test the helper with ordinary environment patching. For example, set the variable with a typed monkeypatch: pytest.MonkeyPatch fixture and assert telemetry.get_nemo_deployment_type() == DeploymentTypeEnum.UNDEFINED. This exercises both the environment read and normalization without reloading the module. The helper does not need to be re-exported as a top-level package API.

What Looks Good

• The normalization helper is small, explicit, and preserves the existing default and case-insensitive valid-value behavior.
• Mapping unknown client-provided values to the existing UNDEFINED enum avoids leaking configuration errors from a best-effort telemetry path.
• The added test targets the unknown-value behavior explicitly.

Verdict

Needs changes — please replace the module reload with a directly testable get_nemo_deployment_type() helper so the test cannot replace telemetry class identities in the shared pytest process.

---

This review was generated by an AI assistant.