Skip to content
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

feat: Adds localization column in the alerts table #267

Merged
merged 46 commits into from
Oct 18, 2023
Merged

feat: Adds localization column in the alerts table #267

merged 46 commits into from
Oct 18, 2023

Conversation

frgfm
Copy link
Member

@frgfm frgfm commented Jul 13, 2023

This PR introduces the following modifications:

  • adds a localization field in the alerts table. Meant to be used as a JSON dump of a list of bounding boxes
  • updates tests
  • updates client
  • adds alembic migration

Here is the new database UML:
uml

cc @MateoLostanlen

@frgfm frgfm added type: improvement New feature or request type: feat labels Jul 13, 2023
@frgfm frgfm added this to the 0.2.0 milestone Jul 13, 2023
@frgfm frgfm requested a review from a team July 13, 2023 07:41
@frgfm frgfm self-assigned this Jul 13, 2023
@ghost
Copy link

ghost commented Jul 13, 2023

Nice of you to open a PR 🙏
When you're ready and want to get it reviewed, post a comment in this Pull Request with this message: /quack review

Copy link
Contributor

@blenzi blenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this addition @frgfm ! We initially thought this was urgent but in the end I propose we take the time to fix the issues with the CI and we deploy a data model consistent with all the other tables and classes:

  • introduce a class Bbox, ROI / RegionOfInterest or equivalent with x0, y0, x1, y1, confidence, and a list of those to AlertIn, instead of strings
  • add a table with the info above and alert_id, to allow multiple bboxes for each alert

I also managed to run the API and the tests locally. My attempts with the CI did not work either so we can try to apply the various changes one by one in another PR:

  • python 3.8 -> 3.9
  • pydantic 1.10 -> 2
  • code-quality fixes
  • postgres 12 -> 15
  • changes in docker-compose, including health-checks

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #267 (9ad74a9) into main (ad657f6) will decrease coverage by 0.47%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
- Coverage   95.18%   94.71%   -0.47%     
==========================================
  Files          66       66              
  Lines        1577     1590      +13     
==========================================
+ Hits         1501     1506       +5     
- Misses         76       84       +8     
Flag Coverage Δ
client 90.58% <38.46%> (-9.42%) ⬇️
unittests 94.95% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/app/api/crud/authorizations.py 100.00% <ø> (ø)
src/app/db/__init__.py 100.00% <ø> (ø)
src/app/db/init.py 44.44% <ø> (ø)
src/app/main.py 77.35% <100.00%> (+0.43%) ⬆️
src/app/models/alert.py 95.45% <100.00%> (+0.21%) ⬆️
src/app/schemas/alerts.py 100.00% <100.00%> (ø)
src/app/schemas/base.py 100.00% <100.00%> (ø)
src/app/schemas/devices.py 100.00% <ø> (ø)
src/app/schemas/groups.py 100.00% <ø> (ø)
src/app/schemas/login.py 100.00% <100.00%> (ø)
... and 3 more

@blenzi
Copy link
Contributor

blenzi commented Oct 18, 2023

@frgfm : the minimal changes for this to work are already in #289

@frgfm
Copy link
Member Author

frgfm commented Oct 18, 2023

@frgfm : the minimal changes for this to work are already in #289

Yup I saw, but it was missing the alembic migration & I took care of the rest sequentially to handle all PR in order 😅

@frgfm frgfm merged commit 51fcdb3 into main Oct 18, 2023
14 of 16 checks passed
@frgfm frgfm deleted the bbox branch October 18, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: client module: database module: models module: schemas topic: build Related to build, installation & CI topic: docker topic: docs Improvements or additions to documentation type: feat type: improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants