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

Issue #162: Add endpoint to retrieve pictures associated with an inspection #225

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

snakedye
Copy link
Contributor

@snakedye snakedye commented Dec 18, 2024

Waiting for get_picture_set_pictures to be replaced.

Closes #162

@snakedye snakedye requested a review from a team as a code owner December 18, 2024 19:42
@github-actions github-actions bot added the tests label Dec 30, 2024
app/main.py Outdated
@@ -108,7 +108,7 @@ async def get_inspection(
)

@app.get("/inspections/{id}/pictures", tags=["Inspections"])
Copy link
Member

@k-allagbe k-allagbe Jan 2, 2025

Choose a reason for hiding this comment

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

I suggest get /files/<file_id>.
Because:

  • we also support pdfs I believe
  • we'll later handle files separately from inspections as their own resource
  • get inspections/<id> should return a list of file ids as part of the data. This is probably what we need to work on most.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, the pipeline does support pdfs. I don't think the refactored pipeline @saratavakoli77 and @Endlessflow are working on does. However, the application (frontend+backend) itself doesn't support sending pdfs and neither does the datastore.

The datastore can return a picture_set_id which I think is akin to a folder in a container (might change in the future), @Francois-Werbrouck can correct me on that. If we have that, I don't think we need to return a full list of files.

Copy link
Member

Choose a reason for hiding this comment

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

Still, a broader term than pictures might give us more cushion for later.

In the frontend, we need the images themselves (hence their ids), not so much their container or set. We want to be able to fetch the images from the backend individually, in parallel, etc.. not always as a set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to get @Francois-Werbrouck opinion on this because it's directly linked to the refactoring he's doing. Personally, I can't think of a use case where we'd benefit from fetching images individually. Even if you have the IDs, how do you know which to fetch?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. It's a matter of good design that allows for easy future improvement. In an optimization process in the frontend we might fetch individual images only when required instead of fetching them all at once.

Question for both of you: how complex would it be to implement that?

Choose a reason for hiding this comment

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

how complex would it be to implement that[ get /files/<file_id>] ?

  1. The Datastore handles blob type of object which original types were previously only pictures/images so we defined the functions and object with the name Picture. Handeling pdf files shouldn't be a problem.
  2. We currently do not save the original type of object of the blob received, but it shouldn't be that complex to add.
  3. We currently allow to either fetch a file by id or fetch a folder content.
  4. I think we currently (and in a short future will have refactored ) allow fetching user's transaction content (all the files of an Inspection in Fertiscan's case)
---
title: Datastore Pydantic Models 
---
classDiagram

note for PictureMetadata "What is returned when fetching<br> a folder's content is a List of this"
class PictureMetadata{
UUID picture_id
str link
bytes blob
}

    class Folder {
        UUID id
        str name
        str path
        List~UUID~ children
        List~UUID~ pictures
        Optional~UUID~ parent_id
    }

    class Container {
        UUID id
        str name
        bool is_public
        str storage_prefix
        List~UUID~ group_ids
        List~UUID~ user_ids
        Dict~UUID, Folder~ folders
        str path
    }

class Client {
        UUID id
        str name
        bool is_public
        str storage_prefix
        Dict~UUID, Container~ containers
    }

Loading

get inspections/ should return a list of file ids as part of the data. This is probably what we need to work on most.

This soon will be answered but I dont see the necessity.
I dont get why we would want to make it more complex, when will we want for an Inspection to partially fetch the files of its list? If the BE wants to include parallelism, you can simply fetch the files individually.

we'll later handle files separately from inspections as their own resource

My opinion on the back end requesting just a bunch of files (pictures) by ID doesn't suit a need in the user-application, but I see some utility for our Data Analyst. However at this point I don't see much urgency in allowing fetching a bunch of files just by IDs with no regards for their placement.
I think sticking to either fetching one file or the entire folder content would be better for the Overall Datastore and having App specific fetching are what's most important at the moment on the application when fetching the files.
If the need for our DataAnalyst to fetch N files by IDs arise because they scraped the DB for specific files, I think it wont be that complex or time consuming to implement it at that point.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I'm putting this into formal request for this issue. In the frontend I need to obtain the file ids associated with an inspection. You guys decide how you want to implement it. I don't mind receiving a set / container id in the inspection data but the backend must allow me to get the individual file ids somehow, which will then be used to query the file data directly, however seen fit.

@k-allagbe

This comment was marked as resolved.

@snakedye snakedye changed the title Issue #165: Add endpoint to retrieve pictures associated with an inspection Issue #162: Add endpoint to retrieve pictures associated with an inspection Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a dev, I want to add endpoints to retrieve the correct label images associated with an inspection
3 participants