python(feat): annotations resource - #792
Brandon-Shippy wants to merge 8 commits into
Conversation
|
Python docs preview: https://sift-stack.github.io/sift/python/pr-792/ Deployed from |
| if state: | ||
| filter_parts.append(cel.equals("state", state.to_filter_str())) | ||
| if assigned_to: | ||
| filter_parts.append(cel.equals("assignee", assigned_to)) |
There was a problem hiding this comment.
assignee filters on the user's name (azimuth maps it to users.name) but the docstring says user id
There was a problem hiding this comment.
docstring for assigned_to now says it filters on the user's name
| ) | ||
| return self._apply_client_to_instance(log) | ||
|
|
||
| async def record_assignment(self, annotation: str | Annotation, user: str) -> AnnotationLog: |
There was a problem hiding this comment.
Drop record_assignment and record_state. They write a "resolved" or "assigned" entry to the feed without changing the annotation, and the server already writes those entries itself whenever resolve/flag/reopen/assign run, so the only thing they add is history that disagrees with the annotation itself
Comments stay since that's intended
There was a problem hiding this comment.
dropped logs.record_assignment and logs.record_state
| ) | ||
| return self._apply_client_to_instance(log) | ||
|
|
||
| async def delete(self, annotation: str | Annotation, log: str | AnnotationLog) -> None: |
There was a problem hiding this comment.
Drop logs.delete, nothing in Sift supports deleting logs, and we shouldn't expose it to users when it's own test says it doesn't work.
| created = await self._low_level_client.create_annotation(create=create) | ||
| return self._apply_client_to_instance(created) | ||
|
|
||
| async def create_review( |
There was a problem hiding this comment.
Let's fold create_review and create_phase into create. Under the hood both just build an AnnotationCreate and call create with annotation_type set, but with renamed fields along the way (assign_to vs assign_to_user_id, channels vs linked_channels, run vs run_id), so users end up having to know two names for everything
client.annotations.create(AnnotationCreate(name="Spike", start_time=start, end_time=end,
assets=["Nostromo"], linked_channels=[AnnotationLinkedChannel(channel_id=c.id_) for c in chans],
assign_to_user_id=bob))
client.annotations.create_review("Spike", start, end, channels=chans, assign_to=bob)
client.annotations.create_phase("Burn", start, end, channels=chans)
Would become:
client.annotations.create(AnnotationCreate(name="Spike", start_time=start, end_time=end,
linked_channels=chans, assign_to_user_id=bob))
client.annotations.create({"name": "Burn", "start_time": start, "end_time": end,
"linked_channels": chans, "annotation_type": AnnotationType.PHASE})
Let AnnotationCreate accept Channel and Asset objects and move the asset-from-channels lookup from _create_typed into create
There was a problem hiding this comment.
folded create_review and create_phase into one create() that takes either AnnotationCreate or PhaseCreate
| """ | ||
| return self.update({"assigned_to_user_id": user}) | ||
|
|
||
| def resolve(self) -> Annotation: |
There was a problem hiding this comment.
Convention is that instance methods forward to the resource and _update (see archive right above). assign/resolve/flag/reopen restate the logic instead and duplicate _set_state, so let's have them call self.client.annotations.resolve(annotation=self) etc., which also deletes Annotation._set_state.
There was a problem hiding this comment.
instance now forwards to the resource, then _update. Annotation._set_state is deleted
| """ | ||
| request = ArchiveAnnotationRequest(annotation_id=annotation_id) | ||
| await self._grpc_client.get_stub(AnnotationServiceStub).ArchiveAnnotation(request) | ||
| return await self.get_annotation(annotation_id) |
There was a problem hiding this comment.
ArchiveAnnotationRequest and UnarchiveAnnotationRequest already return the updated annotation so the extra get_annotation here is redundant
There was a problem hiding this comment.
removed the get_annotation call
| run_id: str | None | ||
| assigned_to_user_id: str | None | ||
| created_by_rule_condition_version_id: str | None | ||
| legend_config: str | None |
There was a problem hiding this comment.
this is not user-useful, drop this field
There was a problem hiding this comment.
| """ | ||
| return self.update({"assigned_to_user_id": user}) | ||
|
|
||
| def resolve(self) -> Annotation: |
There was a problem hiding this comment.
The enum names do not actually correlated to what they are referred to in the application. Use the application nomenclature instead.
There was a problem hiding this comment.
renamed to match open / failed / accepted
There was a problem hiding this comment.
The enums read right now. test_record_state (test_annotations.py:248-253) still uses AnnotationLogState.FLAGGED and the removed logs.record_state, so it fails the first time anyone runs it; delete it. The set_* docstrings at resources/annotations.py:469-494 still say resolve, flag and reopen, so reword them to accepted, failed and open and regenerate the stubs.
Generated by Claude Code
There was a problem hiding this comment.
| start_time: datetime | ||
| end_time: datetime | ||
| annotation_type: AnnotationType = AnnotationType.DATA_REVIEW | ||
| assets: list[str] | None = None |
There was a problem hiding this comment.
asset and run should accept both Object and Id
There was a problem hiding this comment.
assets takes str | Asset
run_id takes str | Run
There was a problem hiding this comment.
assets now takes Asset objects, but a string is still read as an asset name (annotation.py:336-353), while list_(assets=...) reads it as an ID (resources/annotations.py:285-287). Strings should be asset IDs here, like for any other Sift resource, since a name can change. Resolve the IDs to names in create() with the same assets.list_(asset_ids=...) call that _assets_for_channels already makes.
Generated by Claude Code
There was a problem hiding this comment.
create() resolves them to names using _asset_names
| @model_validator(mode="after") | ||
| def _validate_state(self): | ||
| """Phase annotations have no review state; the server rejects one.""" | ||
| if self.annotation_type is AnnotationType.PHASE and self.state is not None: |
There was a problem hiding this comment.
It may be nicer to have different models for the different types since they are very different in usage.
There was a problem hiding this comment.
They are now split into both AnnotationCreate and PhaseCreate but are folded under create()
There was a problem hiding this comment.
The split is in, but annotation_type is still settable on both models (annotation.py:373, 399), so AnnotationCreate(annotation_type=AnnotationType.PHASE, state=...) gets all the way to the server, which rejects a phase with a state. Add a pydantic check so it fails validation instead: pin the type per class (Literal[AnnotationType.PHASE] on PhaseCreate) or bring back a _validate_state model validator. Then move test_create_phase to PhaseCreate and fix test_phase_model_has_no_usable_state, which raises KeyError at head.
Generated by Claude Code
There was a problem hiding this comment.
a mismatched type fails validation now
|
|
||
| text: str | None = None | ||
| user_id: str | None = None | ||
| user_email: str | None = None |
There was a problem hiding this comment.
not a fan of this pattern - we have a User resource now. Can we instead use user_id and a user prop?
There was a problem hiding this comment.
makes sense, done
There was a problem hiding this comment.
user_email is gone, but there's still no user prop: AnnotationCommentElement (annotation.py:448-483) is a plain BaseModel, so reading a mention means calling client.users.get by hand, and writing one rejects a User. Accept str | User for user_id, and resolve mentions through a property on AnnotationLog, which has the client.
Generated by Claude Code
There was a problem hiding this comment.
AnnotationLog.mentioned_users resolves both str and User
| return f"ANNOTATION_STATE_{self.name}" | ||
|
|
||
|
|
||
| class AnnotationLinkedChannel(BaseModel): |
There was a problem hiding this comment.
I don't think we should use a new type for this. Simply reference the existing types.
linked_channels: list[Channel | CalcualtedChannel | ....]
There was a problem hiding this comment.
AnnotationLinkedChannel deleted.
writes do list[Channel | CalculatedChannel]
There was a problem hiding this comment.
Create works, but AnnotationUpdate(linked_channels=...) is silently dropped: exclude=True (annotation.py:301-303) keeps it out of the mask, and update_annotation never adds it back, though the update docstring says it's replaced. Add it to the proto and mask in to_proto_with_mask, as ReportTemplateUpdate does (report_template.py:285-290). On read, line 224 passes calculated-channel version IDs as calculated_channel_id, so resolve them with one GetCalculatedChannelVersions call instead, and fix test_annotations.py:300, since Channel has no channel_id.
Generated by Claude Code
There was a problem hiding this comment.
now sets and adds linked_channels to the mask
There was a problem hiding this comment.
list_versions filters on calculated_channel_version_id now
There was a problem hiding this comment.
test needed to use channel_id like you said
| name_contains: str | None = None, | ||
| name_regex: str | re.Pattern | None = None, | ||
| # self ids | ||
| annotation_ids: list[str] | None = None, |
There was a problem hiding this comment.
should be Annotation | str
There was a problem hiding this comment.
There was a problem hiding this comment.
should be fixed, the docstring was still listed
| pending: bool | None = None, | ||
| assets: list[Asset] | list[str] | None = None, | ||
| runs: list[Run] | list[str] | None = None, | ||
| rule_ids: list[str] | None = None, |
There was a problem hiding this comment.
support Rule and Report objects
There was a problem hiding this comment.
rule_ids and report_ids became rules and replorts which both accept objects or ids
There was a problem hiding this comment.
The objects work now. The list_ docstring still lists rule_ids and report_ids at resources/annotations.py:237-238, and the stub repeats them, so hover docs show two arguments that don't exist. Drop both lines and regenerate the stubs.
Generated by Claude Code
| self, | ||
| *, | ||
| name: str | None = None, | ||
| names: list[str] | None = None, |
There was a problem hiding this comment.
We should combine these into a single arg (not just here but everywhere).
e.g. name: str | list[str] | None
There was a problem hiding this comment.
all have been collapsed
i noticed a pattern on resources wehere we have asset and assets. are these things we want merged as well. it is in the asset resource
There was a problem hiding this comment.
Yes, and let's make it library-wide: please file a ticket to move every list_ (and _build_name_cel_filters) onto the single name: str | list[str] shape and settle pairs like asset/assets the same way, and link it here. Also, a tuple or set in name falls through both checks at resources/annotations.py:256-257 and silently drops the filter, so I'd move that split into _build_name_cel_filters and treat any non-string iterable as a list.
Generated by Claude Code
There was a problem hiding this comment.
now any non-string iterable can be treated as a list
181876f to
b8fcb31
Compare
| start_time: datetime | ||
| end_time: datetime | ||
| assets: list[str | Asset] | None = None | ||
| run_id: str | Run | None = None |
There was a problem hiding this comment.
yeah that makes more sense here
This branch has not been deployed
3 participants
Description
Adds an annotations resource to
sift_client, covering data reviews, phases, and the review history.Annotation and all associated classes and enums are made.
Implements get, list_, find, create, update, assign_to_user, set_open, set_failed, set_accepted, archive/unarchive, add_comment
Validation