pyronear
refactor(models): implement the new data model
Following up on #304, this PR introduces the following modifications:
-
data model
-
update all routes
-
update dependencies, the end to end test, the CI
-
simplify docker orchestration by using localstack by default
-
noticeable changes: the camera aren't "users" anymore, but an admin can create an unexpiring token for cameras (that means they can use a created token, but don't have credentials to log in). This reduces API calls & problems with creds
The only things missing are:
- [ ] Updating the tests
- [ ] Updating the client
In the following PRs, this is what will be added:
- wildfire: aggregation of corresponding detection along time periods & specific geographic areas.
- region/scope : limit what users can read depending on their spatial scope
- Webhooks: to enable easily subscribable webhooks for detection creation, and their confirmation.
Any feedback is welcome!
Hi @frgfm, Love this PR! It will make life easier for us and for pyronear newbies as well !
Just a few things:
To anticipate the use of camera ptz it would be preferable to have a tuple for azimuth in the camera table and to have an azimuth entry (float this time) in the detection table.
Localization is missing from the detection table too.
I'm not sure how you intend to associate a camera list with a user (sdis 7 for example). What do you have planned?
To anticipate the use of camera ptz it would be preferable to have a tuple for azimuth in the camera table and to have an azimuth entry (float this time) in the detection table.
Could you remind me of how is a camera ptz different and why the tuple?
Localization is missing from the detection table too.
It's by design: considering that at this stage, we report a detection from a camera, but we don't have more. But perhaps you mean the bounding box on the image itself? If so, I agree that we need to add it. Perhaps in the next PR to keep this one simple? Would a keypoint be enough? (I'm thinking about the usefulness, and the bounding box is linked to the type of model we use, but it's far from comprehensive, a keypoint would perhaps be more robust for the data model?)
I'm not sure how you intend to associate a camera list with a user (sdis 7 for example). What do you have planned?
cf. the region/scope followup PR I mentioned in the description. Hesitating on the name, but I think "organizations" would work:
- table organization: id, name
- changes to other tables: add a "organization_id" to the user, and camera tables
Then the rest of the logic is easy to implement, and that doesn't complicate things too much
Basically, ptz cameras can be oriented along 3 axes. In our case, both our cameras and those of the Firefighters will patrol N predefined positions. It would therefore be interesting to have these N positions in the form of a tuple (or list of size N). Then, when the cam sends an alert, we'll add the azimuth to the detection table to find out the corresponding position. For statis cams, we'll have a list / tuple of size 1 that will always be equal to the value sent to the detection table.
Yes, I'm talking about the bounding box, which is called localization in the current table, but I agree that the name can be changed. I suggest you keep what we have at the moment, a string that allows you to send any form of points (bbox, keypoints etc ...)
Ok understood for the organization
Yes, don't worry about adding the bbox in a next pr as long as it's present when you deploy it. It's essential information for firefighters to speed up treatment time.
Codecov Report
Attention: Patch coverage is 88.74296% with 60 lines in your changes missing coverage. Please review.
Project coverage is 88.00%. Comparing base (
767be30) to head (44506bf).
Additional details and impacted files
@@ Coverage Diff @@
## main #322 +/- ##
==========================================
- Coverage 94.54% 88.00% -6.54%
==========================================
Files 63 28 -35
Lines 1594 642 -952
==========================================
- Hits 1507 565 -942
+ Misses 87 77 -10
| Flag | Coverage Δ | |
|---|---|---|
| backend | 87.47% <88.56%> (?) |
|
| client | 95.34% <94.11%> (?) |
|
| unittests | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Here are the last edits:
- azimuth field moved from camera table to detection
- updated the client
- updated all the tests
The test suite is considerably faster than before :sweat_smile:
Suggestions for review considering the size of the PR (most of the diff is from the poetry.lock though):
- check the UML in the PR description to address data model concerns (as mentioned, follow-up PRs will polish this, the heavylifting has been done in this PR)
- pull the branch,& run the instructions to get the backend up (the docker orchestration has been greatly improved, everything is up and running even offline & fully working), and check the routes in the swagger
- unfortunately, for the sake of time, you'll have to trust the migration script, the test validity from the CI results
- make sure to specify if you see major incompatibilities with this datamodel, this is the important part
Things to add to prevent feature reduction compared to the previous version:
- refine routes for the platform features
- organization: add a dynamic scope for access management (admin have read/write access everywhere, agent have partial read/write over their organization resources, users have read access over their organization resources)
- webhooks: implement the notification mechanism when a detection is created
- wildfire: later on, implement a structured way to access temporal data about wildfires.