Skip to content

refactor(BA-7722): mark unsent request fields with pydantic MISSING - #14312

Merged
fregataa merged 3 commits into
mainfrom
BA-7722-unset-sentinel
Sep 7, 2026
Merged

refactor(BA-7722): mark unsent request fields with pydantic MISSING#14312
fregataa merged 3 commits into
mainfrom
BA-7722-unset-sentinel

Conversation

@HyeockJinKim

Copy link
Copy Markdown
Collaborator

Problem

Update-request DTOs mark an unsent field with a hand-rolled Sentinel enum
(common/api_handlers.py). It serializes as the integer 1, so:

  • The published spec advertises the placeholder. docs/manager/rest-reference/openapi.json
    carries Sentinel as {"type":"integer","enum":[1]}, referenced by 78 fields across 25
    Update*Input schemas, each with "default": 1.
  • The value reaches the wire whenever a caller sets the sentinel explicitly, which the v2 CLI
    does (client/cli/v2/object_storage/commands.py). The server happens to decode 1 back
    into the enum, so this has gone unnoticed.

Change

UNSET is pydantic_core.MISSING. pydantic omits a field holding it from serialization and
from the JSON schema, so an absent field puts nothing on the wire.

                        before                    after
absent            {"time_window": 1}              {}
null              {}  (dropped, see below)        {"time_window": null}
value             {"time_window": "30m"}          {"time_window": "30m"}

JSON schema       anyOf[string, $ref:Sentinel, null], default: 1
                  anyOf[string, null]

Unset aliases typing_extensions.Sentinel rather than the exact MISSING type: mypy merged
PEP 661 support (python/mypy#21647, 2026-08-07) but has not released it. MISSING is a real
instance of that class, so the annotation is wider than needed but never false, and mypy accepts
it with no suppressions. When mypy ships PEP 661, unset.py and the two from_unset bodies
change and nothing else does — common/tristate/KNOWLEDGE.md records the steps.

Conversion

TriState.from_unset / OptionalState.from_unset map the three states onto the target column.
What null means is decided by the column, and the constructor the adapter picks states it.

Column Constructor null unset
nullable TriState.from_unset clears it leaves it
non-nullable OptionalState.from_unset leaves it leaves it

OptionalState.and_optional / and_tri read a nested request model. Nested access is on
OptionalState only: a nested model is a wire-side grouping with no column behind it, so a null
parent must not spread a nullify into its children — a non-nullable child would violate its
constraint and a nullable one would be erased unasked.

The adapter loses its hand-rolled three-way ternaries:

options = OptionalState[ModifyQueryDefinitionOptionsRequest].from_unset(request.options)
return PrometheusQueryPresetUpdater(
    preset_id=PrometheusQueryPresetID(preset_id),
    name=OptionalState.from_unset(request.name),
    time_window=TriState.from_unset(request.time_window),
    filter_labels=options.and_optional(lambda o: o.filter_labels),
    ...
)

Scope

Foundation plus one reference entity (prometheus_query_preset). common/dto/AGENTS.md gains
the create-vs-update schema rules this follows.

Follow-ups, not in this PR:

  • Migrate the remaining 25 DTO modules, then delete the old Sentinel.
  • Drop exclude_none=True from client/v2/base_client.py, which currently discards an explicit
    null and so leaves a client unable to express NULLIFY at all. It must land after every field is
    migrated — removing it earlier would put the integer 1 on the wire more often, not less.

Verification

  • pants check --changed-since=origin/main --changed-dependents=transitive — 7152 source files,
    no issues. This covers the pydantic 2.11.10 → 2.13.5 bump across the repo.
  • pants lint clean, including the new /tristate/** visibility rule.
  • Exercised against the exported venv: the three states resolve as tabled above, the nested
    reads resolve, CreateQueryDefinitionRequest is untouched, and the time_window field
    validator still rejects a bad duration.
  • pants test left to CI per the submit workflow.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UHHR2fFGQNB3NssxHeRKBz

Update-request DTOs marked an unsent field with a hand-rolled `Sentinel` enum.
It serialized as the integer `1`, which reached the wire whenever a caller set it
explicitly and was published in the OpenAPI spec as `{"type":"integer","enum":[1]}`.

`UNSET` is `pydantic_core.MISSING`; pydantic omits a field holding it from both
serialization and the JSON schema. `Unset` aliases `typing_extensions.Sentinel`
rather than the exact `MISSING` type because mypy has merged PEP 661 support but
not released it. `MISSING` is a real instance of that class, so the annotation is
wider than needed but never false, and mypy accepts it with no suppressions.

`TriState.from_unset` and `OptionalState.from_unset` map the three states onto the
target column, and `OptionalState.and_optional` / `and_tri` read a nested request
model. Nested access is on `OptionalState` only: a nested model has no column
behind it, so a null parent must not spread a nullify into its children.

This covers the foundation and one reference entity. Migrating the remaining DTO
modules and dropping `exclude_none` from the v2 client transport follow separately;
the latter must wait until no field is a `Sentinel` any more.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHHR2fFGQNB3NssxHeRKBz
@HyeockJinKim
HyeockJinKim requested a review from a team as a code owner September 7, 2026 06:27
Copilot AI balanced review requested due to automatic review settings September 7, 2026 06:27
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHHR2fFGQNB3NssxHeRKBz
@github-actions github-actions Bot added size:L 100~500 LoC comp:manager Related to Manager component comp:common Related to Common component labels Sep 7, 2026
Co-authored-by: octodog <mu001@lablup.com>
@github-actions github-actions Bot added the area:docs Documentations label Sep 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new serialization and tri-state conversion contracts lack automated regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Replaces the legacy update-field sentinel with Pydantic MISSING, using Prometheus query presets as the reference migration.

Changes:

  • Adds reusable unset and tri-state conversion helpers.
  • Migrates Prometheus update DTOs and adapter logic.
  • Upgrades Pydantic and regenerates lockfiles.
File summaries
File Description
tools/mypy.lock.metadata Updates Pydantic requirement metadata.
tools/mypy.lock Refreshes mypy tool dependencies.
tools/mypy-requirements.txt Requires Pydantic 2.13.5.
src/ai/backend/manager/types.py Adds unset and nested-state conversions.
src/ai/backend/manager/api/rest/prometheus_query_preset/adapter.py Uses the new conversion helpers.
src/ai/backend/common/tristate/unset.py Defines UNSET and Unset.
src/ai/backend/common/tristate/KNOWLEDGE.md Documents sentinel rationale and migration.
src/ai/backend/common/tristate/AGENTS.md Adds tri-state usage guardrails.
src/ai/backend/common/tristate/__init__.py Initializes the package.
src/ai/backend/common/dto/manager/prometheus_query_preset/request.py Migrates update fields to UNSET.
src/ai/backend/common/dto/AGENTS.md Documents create/update schema rules.
src/ai/backend/common/BUILD Adds tristate dependency boundaries.
requirements.txt Upgrades runtime Pydantic.
python.lock.metadata Updates lock requirement metadata.
python.lock Regenerates runtime dependencies.
changes/14312.enhance.md Adds the changelog entry.
Review details
  • Files reviewed: 13/17 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +75 to +80
name: str | None | Unset = Field(default=UNSET, description="Human-readable name")
metric_name: str | None | Unset = Field(default=UNSET, description="Prometheus metric name")
query_template: str | None | Unset = Field(
default=UNSET, description="PromQL template with placeholders"
)
time_window: str | Sentinel | None = Field(default=SENTINEL, description="Default time window")
options: ModifyQueryDefinitionOptionsRequest | None = Field(
default=None, description="Query definition options"
time_window: str | None | Unset = Field(default=UNSET, description="Default time window")
Comment on lines +284 to +299
def and_optional[TNew](self, fn: Callable[[TVal], TNew | None | Unset]) -> OptionalState[TNew]:
"""Read a field of the held value onto a non-nullable column.

For nested request models. An absent parent yields nop.
"""
if self._state == _TriStateEnum.UPDATE:
return OptionalState.from_unset(fn(self.value()))
return OptionalState.nop()

def and_tri[TNew](self, fn: Callable[[TVal], TNew | None | Unset]) -> TriState[TNew]:
"""Read a field of the held value onto a nullable column.

An absent parent yields nop; only the field's own null nullifies.
"""
if self._state == _TriStateEnum.UPDATE:
return TriState.from_unset(fn(self.value()))
@fregataa
fregataa merged commit 66c3364 into main Sep 7, 2026
42 checks passed
@fregataa
fregataa deleted the BA-7722-unset-sentinel branch September 7, 2026 07:22
seedspirit pushed a commit that referenced this pull request Sep 7, 2026
…14312)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: octodog <mu001@lablup.com>
seedspirit added a commit that referenced this pull request Sep 7, 2026
BA-7722 (#14312) replaced the SENTINEL marker for an unsent update field with
pydantic MISSING, wrapped as Unset/UNSET, and made it the rule for update schemas
in common/dto/AGENTS.md. Move UpdateRuntimeVariantPresetInput onto it rather than
land two new fields on the mechanism that was just retired.

Reading every field through TriState.from_unset / OptionalState.from_unset also
drops the three-way ternary each one needed, taking the adapter's update from 47
lines to 15.

Dropping default=None from the GQL input fixes a data loss on the GraphQL path:
strawberry filled an omitted field with null, and null on a nullable column means
clear, so updating only the rank wiped the preset's description and default_value.
An omitted field is now strawberry UNSET, which to_pydantic skips. The fields stay
optional in the schema and an explicit null still clears them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
seedspirit added a commit that referenced this pull request Sep 7, 2026
BA-7722 (#14312) replaced the SENTINEL marker for an unsent update field with
pydantic MISSING, wrapped as Unset/UNSET, and made it the rule for update schemas
in common/dto/AGENTS.md. Move UpdateRuntimeVariantPresetInput onto it rather than
land two new fields on the mechanism that was just retired.

Reading every field through TriState.from_unset / OptionalState.from_unset also
drops the three-way ternary each one needed, taking the adapter's update from 47
lines to 15.

Dropping default=None from the GQL input fixes a data loss on the GraphQL path:
strawberry filled an omitted field with null, and null on a nullable column means
clear, so updating only the rank wiped the preset's description and default_value.
An omitted field is now strawberry UNSET, which to_pydantic skips. The fields stay
optional in the schema and an explicit null still clears them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
seedspirit added a commit that referenced this pull request Sep 7, 2026
BA-7722 (#14312) retired the SENTINEL marker for an unsent update field in favour
of pydantic MISSING, wrapped as Unset/UNSET, and common/dto/AGENTS.md now requires
it of update schemas. Declare the two fields this branch adds that way rather than
land them on the mechanism that was just replaced.

Only the new fields move. The eleven that were already on SENTINEL keep it, and
none of the model validators read the new fields, so no existing default or code
path changes. Converting the rest is a follow-up.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
seedspirit added a commit that referenced this pull request Sep 8, 2026
BA-7722 (#14312) made Unset/UNSET the marker for a field an update request did not
send, and common/dto/AGENTS.md now requires it of update schemas. The patch inputs
this branch adds are update schemas, so they carry it rather than a None default.

Behaviour is unchanged: the merge already keyed off model_fields_set through
model_dump(exclude_unset=True), and the GQL inputs already defaulted to strawberry
UNSET. What changes is the published contract -- an omitted field no longer reads
as "defaults to null" in the OpenAPI, which is what "omit to keep the current
value" meant all along.

A union holding Unset drops a field-level constraint from the generated schema, so
the six constraints ride on their annotated member instead. Verified that
exclusiveMinimum, minimum, minLength and maxItems all survive, the last of which
would otherwise have been emitted as maxLength on an array.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
seedspirit added a commit that referenced this pull request Sep 8, 2026
BA-7722 (#14312) retired the SENTINEL marker for an unsent update field in favour
of pydantic MISSING, wrapped as Unset/UNSET, and common/dto/AGENTS.md now requires
it of update schemas. Declare the two fields this branch adds that way rather than
land them on the mechanism that was just replaced.

Only the new fields move. The eleven that were already on SENTINEL keep it, and
none of the model validators read the new fields, so no existing default or code
path changes. Converting the rest is a follow-up.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
seedspirit added a commit that referenced this pull request Sep 9, 2026
BA-7722 (#14312) retired the SENTINEL marker for an unsent update field in favour
of pydantic MISSING, wrapped as Unset/UNSET, and common/dto/AGENTS.md now requires
it of update schemas. Declare the two fields this branch adds that way rather than
land them on the mechanism that was just replaced.

Only the new fields move. The eleven that were already on SENTINEL keep it, and
none of the model validators read the new fields, so no existing default or code
path changes. Converting the rest is a follow-up.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:docs Documentations comp:common Related to Common component comp:manager Related to Manager component size:L 100~500 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants