Fix fetch unlabeled detections endpoint #366
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
- Coverage 86.39% 86.31% -0.09%
==========================================
Files 34 34
Lines 941 950 +9
==========================================
+ Hits 813 820 +7
- Misses 128 130 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -165,19 +165,24 @@ async def fetch_unlabeled_detections( | |||
| def get_url(detection: Detection) -> str: | |||
| return bucket.get_public_url(detection.bucket_key) | |||
|
|
|||
| async def get_url_with_bucket(detection: Detection) -> str: | |||
| camera = cast(Camera, await cameras.get(detection.camera_id, strict=True)) | |||
There was a problem hiding this comment.
Considering the size of the potential result, this could become quite slow. We've had to solve this on the old-production branch. I suggest taking the same approach : https://github.com/pyronear/pyro-api/blob/old-production/src/app/api/endpoints/events.py#L149
(otherwise if there are 150 detections, we'll have to do 150 invidual camera fetch)
There was a problem hiding this comment.
ok I have implemented a pagination parameter in the endpoint (and the fetch_all function) is it ok for you ?
There was a problem hiding this comment.
On paper yes, but what is implemented on old-production was proofed with platform interactions and extremely fast. Pagination will for sure needed at some point, but for now, for the sake of "let's do what we know works well", I'd suggest taking only adapting that code. If you feel like we should introduce pagination now:
- add a param to change the num of events (https://github.com/pyronear/pyro-api/blob/old-production/src/app/api/endpoints/events.py#L165), but enforce that it cannot be bigger than 15 for instance
- add another param to change the num of consecutive alerts per event for the GIF (https://github.com/pyronear/pyro-api/blob/old-production/src/app/api/endpoints/events.py#L221), but enforce that it cannot be bigger than 10 for instance
Considering this is only for the platform and for now, it fills that need, I think those param and pagination aren't necessary but I'll let you be the judge of that 👌
There was a problem hiding this comment.
Thanks! I'm pretty sure everything here works but as mentioned in the comment, I have no clue how fast. This route needs to be really low latency and we've iterated on old-production branch to make something that works. Out of safety, I think we should keep that design 👌
| @@ -165,19 +165,24 @@ async def fetch_unlabeled_detections( | |||
| def get_url(detection: Detection) -> str: | |||
| return bucket.get_public_url(detection.bucket_key) | |||
|
|
|||
| async def get_url_with_bucket(detection: Detection) -> str: | |||
| camera = cast(Camera, await cameras.get(detection.camera_id, strict=True)) | |||
There was a problem hiding this comment.
On paper yes, but what is implemented on old-production was proofed with platform interactions and extremely fast. Pagination will for sure needed at some point, but for now, for the sake of "let's do what we know works well", I'd suggest taking only adapting that code. If you feel like we should introduce pagination now:
- add a param to change the num of events (https://github.com/pyronear/pyro-api/blob/old-production/src/app/api/endpoints/events.py#L165), but enforce that it cannot be bigger than 15 for instance
- add another param to change the num of consecutive alerts per event for the GIF (https://github.com/pyronear/pyro-api/blob/old-production/src/app/api/endpoints/events.py#L221), but enforce that it cannot be bigger than 10 for instance
Considering this is only for the platform and for now, it fills that need, I think those param and pagination aren't necessary but I'll let you be the judge of that 👌
| @@ -64,8 +64,7 @@ async def create_detection( | |||
| # guess_extension will return none if this fails | |||
| extension = guess_extension(magic.from_buffer(file.file.read(), mime=True)) or "" | |||
| # Concatenate timestamp & hash | |||
| bucket_key = f"{token_payload.sub}-{datetime.utcnow().strftime('%Y%m%d%H%M%S')}-{sha_hash[:8]}{extension}" | |||
| # Reset byte position of the file (cf. https://fastapi.tiangolo.com/tutorial/request-files/#uploadfile) | |||
| bucket_key = f"{token_payload.sub}-{datetime.utcnow().strftime('%Y%m%d%H%M%S')}-{sha_hash[:8]}{extension}" # Reset byte position of the file (cf. https://fastapi.tiangolo.com/tutorial/request-files/#uploadfile) | |||
There was a problem hiding this comment.
Can you revert the comment position to the next line as previously? (gonna have formatting errors at some point otherwise)
When calling the fetch unlabeled detections endpoint with an ADMIN token, the pictures were not found in the S3 because the organisation_id of ADMIN is = 1, while we would like to get all the Detection of all the organization.
So for ADMIN token we should find the organization_id thanks to the Camera.detection_id