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

Solution to #258 [CLIP][Server/Engine] Send images to engine / accept PIL images #353

Merged
merged 21 commits into from
Sep 18, 2024

Conversation

Gavinfornever
Copy link
Contributor

@Gavinfornever Gavinfornever commented Sep 8, 2024

Hi everyone. I'm a developer using Infinity to build LLM deployment&inference platform and now hope to contribute this wonderful project! I've met this exact issue(#258) before and bypassed it by "image_emb, _ = await self.engine._batch_handler._schedule(images)". But this isn't intuitive or usable when inputs are mixed with urls and Image objects.

So I try to solve this issue with michael's instructions and my ideas. @michaelfeil
23523fdsf

And have basically accomplished the following:

  1. allow engine to use PIL image objects as input to embed.
  2. mixed(of course optional) input of urls and PIL Image objects
  3. multiprocess resolving of imgs
  4. exception handles hopefully to ensure safety
  5. change some annotations

Questions:

  1. "Allow API to download images", does this API mean the pic below? cause PIL.Image.Image objects cannot be passed through http directly. I'm not clear about this, so let's discuss more!
    I suggest adding bytes stream input to this api. In my familiar situations, images are stored and shared among modules to reduce redundance of downloading; or just pass imgs using bytes stream among apis.
Pasted image 20240908225804 2. **How can I get started with testing of this draft pr?** I'll be follow the test process while waiting for some hints from you guys!

@michaelfeil
Copy link
Owner

@Gavinfornever Thanks for your efforts! Short comment here. I think its slightly more complicated. Potentially, you need two fields, file_upload for pngs and list of urls, as two variables (makes ordering complicated).

Roughly the server side code:

async def embed_image(
    ...
    upload_images: UploadFile, # maybe also list[images]
    input: list[url]
    ...
):
    ....
    images= await upload_images.read()
    ....

Roughly Client code for that example:

    async with aiohttp.ClientSession() as session:
            form_data = aiohttp.FormData()

            form_data.add_field(
                content_type="application/png",
                filename="dummy.png",
                name="upload_image",
                value=file,
            )
            form_data.add_field(name="input", value=[url1,url2])
            ... # more form data if needed.

            async with session.post(
                url=f"localhost:7997/v1/image_embed/",
                data=form_data,
            ) as response:
                response.raise_for_status()
                json_response = await response.json()

More:
https://fastapi.tiangolo.com/tutorial/request-files/

@michaelfeil
Copy link
Owner

I would say, break this PR up into two.

First PR:

  • works on `engine.image_embed(images: Pil.Image | url)
  • no work on FastAPI

Second PR:

  • only work on FastAPI to accept the image and transform it into the PIL image, including downloading / checking utils

@Gavinfornever
Copy link
Contributor Author

Gavinfornever commented Sep 9, 2024

@michaelfeil
Hi, I have passed the test workflow in my fork, like below. And the latest code has been updated in this draft, you can run the test flow again and hopefully code review can be ready!

FYI, in the test, poetry made me modify the import format of typing in async_engine.py(which is odd cause in other places import is not that strict). Otherwise, it cannot pass the make lint # 3.12.

Last, this pr achieve image inputs to Engine ONLY but for Server it will be accomplished in future pr like michael said above.

Copy link
Owner

@michaelfeil michaelfeil left a comment

Choose a reason for hiding this comment

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

Overall nice work.

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 73.68421% with 10 lines in your changes missing coverage. Please review.

Project coverage is 77.97%. Comparing base (65afe2b) to head (9a1be05).

Files with missing lines Patch % Lines
libs/infinity_emb/infinity_emb/primitives.py 28.57% 5 Missing ⚠️
...inity_emb/infinity_emb/transformer/vision/utils.py 80.00% 5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
- Coverage   78.03%   77.97%   -0.06%     
==========================================
  Files          35       35              
  Lines        2549     2574      +25     
==========================================
+ Hits         1989     2007      +18     
- Misses        560      567       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michaelfeil michaelfeil marked this pull request as ready for review September 16, 2024 00:40
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request implements support for both URL strings and PIL Image objects as input for image embedding in the Infinity project, addressing issue #258.

  • Added functionality in engine.py and batch_handler.py to handle mixed input types (URLs and PIL Images) for the image_embed method
  • Introduced new types and error handling in primitives.py to support PIL Image objects and improve type checking for image-related classes
  • Modified utils.py to resolve images from both URLs and PIL Image objects, with improved error handling and flexibility
  • Added a new test case test_clip_embed_pil_image_input in test_engine.py to verify PIL Image input functionality
  • Updated sync_engine.py to align with the new image embedding capabilities, supporting mixed input types

7 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings

libs/client_infinity/run_tests_with_hook.sh Outdated Show resolved Hide resolved
libs/infinity_emb/infinity_emb/engine.py Outdated Show resolved Hide resolved
libs/infinity_emb/infinity_emb/transformer/vision/utils.py Outdated Show resolved Hide resolved
Copy link
Owner

@michaelfeil michaelfeil left a comment

Choose a reason for hiding this comment

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

Approved. Before merging, please resolve the comments I left.

libs/infinity_emb/tests/data/imgs/000000039769.jpg Outdated Show resolved Hide resolved
libs/infinity_emb/infinity_emb/sync_engine.py Outdated Show resolved Hide resolved
libs/infinity_emb/infinity_emb/sync_engine.py Outdated Show resolved Hide resolved
libs/client_infinity/run_tests_with_hook.sh Outdated Show resolved Hide resolved
libs/infinity_emb/infinity_emb/transformer/vision/utils.py Outdated Show resolved Hide resolved
@Gavinfornever
Copy link
Contributor Author

Gavinfornever commented Sep 16, 2024

@michaelfeil
Resolved all comments. Set requests and types-requests to 2.28.1 in test group to avoid getting httpResponse(rather than byte) when using .get().raw to get imgs in vision/utils.

@michaelfeil michaelfeil merged commit 585aca1 into michaelfeil:main Sep 18, 2024
36 checks passed
@michaelfeil
Copy link
Owner

Thanks Gao for your contribution, awesome work. Excited for more!

@Gavinfornever
Copy link
Contributor Author

My pleasure! Looking into the benchmark stuff soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants