-
-
Notifications
You must be signed in to change notification settings - Fork 10
Alerts #538
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
base: main
Are you sure you want to change the base?
Alerts #538
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #538 +/- ##
==========================================
- Coverage 83.99% 82.51% -1.48%
==========================================
Files 42 47 +5
Lines 1318 1687 +369
==========================================
+ Hits 1107 1392 +285
- Misses 211 295 +84
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3e18740 to
6ab2d8d
Compare
5d59011 to
761fefd
Compare
4c944fc to
204bbb6
Compare
fe51
left a comment
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.
Hi there, thanks a lot for the PR.
I have started to review it, here are few comments and questions before going deeper.
| pose_id: Union[int, None] = Field(None, foreign_key="poses.id", nullable=True) | ||
| azimuth: float = Field(..., ge=0, lt=360) | ||
| is_wildfire: Union[AnnotationType, None] = None | ||
| cone_azimuth: Union[float, None] = Field(None, nullable=True) |
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.
Here cone_azimuth is what is expected to be the azimuth of the sequence right ? Does sequence_azimuth sounds good to you ? We must find something as clear as possible to avoid consfusion
| azimuth: float = Field(..., ge=0, lt=360) | ||
| is_wildfire: Union[AnnotationType, None] = None | ||
| cone_azimuth: Union[float, None] = Field(None, nullable=True) | ||
| cone_angle: Union[float, None] = Field(None, nullable=True) |
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.
Should not we go for angular_width? Also, this one must be in degree, hence a suggestion below
| cone_angle: Union[float, None] = Field(None, nullable=True) | |
| angular_width: Union[float, None] = Field(None, ge=0, lt=360, nullable=True) |
(modification must be done elsewhere to adapt the naming)
|
|
||
| async def get_session() -> AsyncSession: # type: ignore[misc] | ||
| async_session = sessionmaker(bind=engine, class_=AsyncSession, expire_on_commit=False) | ||
| async_session = async_sessionmaker(bind=engine, class_=AsyncSession, expire_on_commit=False) |
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.
Naive question: why do we need to switch to asynchronous approach here?
|
|
||
|
|
||
| class AlertUpdate(BaseModel): | ||
| organization_id: Optional[int] = Field(None, gt=0) |
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.
As AlertCreate and Alertupdate have properties in common, you might create a AlertBase schemas to put shared properties (org id, lat lon, started_at, last_seen_at might be deferent if optinnal in one case but not the other)
| class Alert(SQLModel, table=True): | ||
| __tablename__ = "alerts" | ||
| id: int = Field(None, primary_key=True) | ||
| organization_id: int = Field(..., foreign_key="organizations.id", nullable=False) |
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.
| organization_id: int = Field(..., foreign_key="organizations.id", nullable=False) |
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.
Why organization_id is needed here ? Shouldn't we handle it like we do for sequences, and cross-check via the user's scope?
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.
We need it to filter alerts, when you fetch alerts using fetch_latest_unlabeled_alerts for exemple you want alerts from your organisation only
| telemetry_client.capture( | ||
| token_payload.sub, event="organizations-deletion", properties={"organization_id": organization_id} | ||
| ) | ||
| # Remove alerts and their associations for this organization to satisfy FK constraints |
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.
As mentioned in the model comments -> if the organisationid is not required in alerts, this part of the code must therefore be deleted.
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.
same as previous comment
|
|
||
|
|
||
| class AlertCreate(BaseModel): | ||
| organization_id: int = Field(..., gt=0) |
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.
org_id might not be needed in alert db model, hence not useful in schema
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.
same
| ) | ||
|
|
||
| # Ensure the newly created sequence is present | ||
| if all(seq.id != sequence_.id for seq in recent_sequences): |
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.
Naive question, in which case the sequence could be missing ?
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.
when you create a new sequence
| "lat": float(cam.lat), | ||
| "lon": float(cam.lon), | ||
| "cone_azimuth": float(seq.cone_azimuth), | ||
| "cone_angle": float(seq.cone_angle), |
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.
(maybe to) Rename according to what is chosen as the attribute name in the database model.
This PR introduces an alerts feature on the API side, including the data model, schemas, CRUD layer, and endpoints. It also integrates alerts into the existing detections and sequences flows.
What changed
Alerts API
New alerts endpoint
src/app/api/api_v1/endpoints/alerts.pyNew alert domain layer
src/app/crud/crud_alert.pysrc/app/schemas/alerts.pysrc/app/models.pyRouter and dependency wiring updated accordingly
Sequences and detections
sequences.pyanddetections.pystarted_at,last_seen_at)Overlap logic
src/app/services/overlap.pyScope clarification, intentional follow up
pose_idis plannedpose_idbased split will be introduced in a separate follow up PRHow to test
Using the dev environment
https://github.com/pyronear/pyro-envdev
Notebook example
http://0.0.0.0:8889/notebooks/notebooks/send_real_alerts.ipynb