lablup / lablup/backend.ai

Drop the id conversion hooks from the field owner lookup specs and let the column type convert

Open
#13,947 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
670
Forks
183
Avg merge
15h 13m
Merged PRs (30d)
368

Description

The field owner lookup specs each declare a conversion hook that no longer converts anything. Owner columns are declared as GUID(OwnerID), and GUID.process_result_value already applies the subtype, so a row comes back holding the entity id class it should. Twelve of the thirteen implementations therefore re-wrap a value that is already the right type.

The hook also weakens the declaration it appears to strengthen. Its signature takes a bare UUID, unrelated to whatever column build_query actually selected, so selecting the wrong column still yields a correctly named value. The declared Select[tuple[TFieldID, TOwnerID]] is what should hold, and the column type is what can hold it.

Scope:

- Remove FieldOwnerLookup.to_entity_id, FieldOwnerKeyLookup.to_entity_id, and FieldKeyLookup.to_entity_id/to_field_id from models/specs/lookup.py.
- Use the row values directly at the four call sites in repositories/ops/v2/read.py (lines 91, 104, 118, 132).
- Cast kernels.session_id to GUID(SessionID) in models/kernel/lookups.py. It is the only implementation whose hook does real work: the column is SessionIDColumnType, which yields SessionId (a NewType over UUID), while the lookup declares SessionID (an EntityIdentifier subclass). No migration is needed.
- Delete the now-empty implementations in the lookups.py of error_log, artifact_revision, keypair, endpoint, login_session, model_card, deployment_revision_preset, and rbac_models/role_permission_preset.
- Update models/specs/AGENTS.md and KNOWLEDGE.md where they describe the conversion hook.

Out of scope: DataLookup.to_entity_id(row). That one reads a whole row, so something has to say which column is the id; the roughly twenty implementations of it stay as they are.

Ordering: this touches files that BA-7460 and BA-7463 relocate wholesale, so it runs after them and absorbs the rebase in one place. Specs relocated in the meantime may arrive carrying the hook; this issue sweeps them out together.

Success Criteria
- [ ] models/specs/lookup.py declares build_query as the only abstract method on the three field owner lookup specs
- [ ] no implementation of FieldOwnerLookup, FieldOwnerKeyLookup, or FieldKeyLookup defines to_entity_id or to_field_id
- [ ] lookup_field_owners on a keypair id returns a UserID instance, not a bare UUID
- [ ] lookup_field_owners on a kernel id returns a SessionID instance, not a SessionId
- [ ] lookup_field_by_key on an access key returns a KeyPairID and a UserID instance
- [ ] a lookup selecting a column whose type is not GUID(OwnerID) fails the type check rather than silently returning a bare UUID
- [ ] pants check and pants lint pass
- [ ] pants test passes for affected packages

JIRA Issue: BA-7478

Contributor guide

Open the contributing guide

Research direction

Start with models/specs/lookup.py, the four call sites in repositories/ops/v2/read.py, and the kernel lookup in models/kernel/lookups.py; review the documented hook behavior in models/specs/AGENTS.md and KNOWLEDGE.md. Remove the field-owner conversion hooks and empty implementations named in the issue, preserving the kernel type conversion. Verify the listed type-checking success criteria, then run pants check, pants lint, and affected-package tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.