-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Annotations #383
Annotations #383
Conversation
A preview of bcd21d7 is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/383 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
TODO, will add more:
|
647ec4f
to
76caa91
Compare
76caa91
to
2df6cdb
Compare
2df6cdb
to
68f590d
Compare
1. Exposes as mixin in the BE 2. Adds GET/POST/PUT endpoints 3. Creates data models Does not work with s3 yet.
68f590d
to
ad6b2fe
Compare
This allows for: 1. Creating + editing annotations in the app view 2. Viewing all annotations for a project 3. Editing in the project view
ad6b2fe
to
bcd21d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to bcd21d7 in 59 seconds
More details
- Looked at
3020
lines of code in25
files - Skipped
1
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. telemetry/ui/src/components/routes/app/AppView.tsx:191
- Draft comment:
Consider simplifying the logic for setting the current action index by consolidating the logic for 'forceCurrentActionIndex' and 'disableNavigateSteps'. This will make the code cleaner and reduce duplication. - Reason this comment was not posted:
Confidence changes required:50%
The code in AppView.tsx has a potential issue with the use of the 'forceCurrentActionIndex' prop. The logic to set the current action index is duplicated and could be simplified.
2. telemetry/ui/src/examples/MiniTelemetry.tsx:22
- Draft comment:
Consider removing the check against the string 'null' for the 'partitionKey' prop if it's already typed as 'string | null'. This can simplify the code and prevent potential errors. - Reason this comment was not posted:
Confidence changes required:30%
In the MiniTelemetry.tsx file, the 'partitionKey' prop is checked against the string 'null', which might not be necessary if the prop is already typed as 'string | null'.
Workflow ID: wflow_TSM3mfjJbSGkk8Mv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good for a v1
Adds annotations workflow. See #285.
High-level:
Changes
How I tested this
Notes
Remaining:
Checklist
Important
Add comprehensive annotations workflow with backend endpoints and frontend views for creating, updating, and managing annotations.
AnnotationsBackendMixin
tobackend.py
for handling annotations.run.py
for creating, updating, and retrieving annotations.AnnotationsViewContainer
andAnnotationsView
inAppView.tsx
for displaying annotations.Drawer
component indrawer.tsx
for annotation editing.ProjectList.tsx
to include annotations link.AnnotationCreate
,AnnotationUpdate
,AnnotationOut
,AnnotationObservation
, andAnnotationDataPointer
.DefaultService.ts
with new annotation-related API calls.package.json
dependencies for new features.Chip
component updates inchip.tsx
for annotation tags.This description was created by
for bcd21d7. It will automatically update as commits are pushed.