Skip to content

Add vectorstore image search app#141

Draft
diksipav wants to merge 4 commits intomainfrom
add-vectorstore-image-search-app
Draft

Add vectorstore image search app#141
diksipav wants to merge 4 commits intomainfrom
add-vectorstore-image-search-app

Conversation

@diksipav
Copy link

No description provided.

@diksipav diksipav requested a review from anbuzin February 13, 2025 17:10
readme = "README.md"
requires-python = ">=3.12"
dependencies = [
"gel>=0.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"gel>=0.0.1",
"gel[ai]>=0.0.1",

Copy link
Author

Choose a reason for hiding this comment

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

oh yes, these things I will update at the end... I couldnt even install deps from pyproject.toml, I saw some error about having both app and dbschema in the root level but didn't fix it (didn't even try)... @deepbuzin were u able to install deps for fastapi tutorial, u have the similar folder structure there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remember running into any issues in either case. I'm using uv. You can tell me what to do to reproduce, I'll look into it

Copy link
Author

Choose a reason for hiding this comment

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

@deepbuzin my question is how do you install deps in Fastapi tutorial if you don't install them explicitly mentioning all of them, other examples have a Makefile but since the Fastapi tutorial doesn't have it, I thought maybe is not mandatory?

For users that just want to clone the example and start using it without following the tutorial and without installing all deps one by one, but want to run one command and install all of them? What is that command in the case when there's no Makefile?

requires-python = ">=3.12"
dependencies = [
"gel>=0.0.1",
"fastapi>=0.109.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"fastapi>=0.109.0",
"fastapi[standard]>=0.109.0",

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if we replaced this CLIP implementation with Transformers. It's better maintained, and would also unlock SigLIP, which is a slightly more modern variation of the same thing.

I'm also in favor of replacing the OpenAI library dep with a raw HTTP request (or better yet, an HTTP request to Ollama to go fully local).

Copy link
Author

Choose a reason for hiding this comment

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

Using openai and clip is easier for the tutorial and there's a bigger chance people are more familiar with them, we can write somewhere in the tutorial that people can also use these other things... I can use http for open ai call (even tho I don't think is a big difference, we also use fastapi for http requests). But ofc if u and others think it'll be better I will update : )

Copy link
Author

Choose a reason for hiding this comment

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

On the other side if Transformers and Ollama are better/more popular ways to do it nowadays, I will update.. I just didn't hear of it and didn't find it when I researched the topic :D

Copy link
Contributor

Choose a reason for hiding this comment

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

I will leave this one up to you :) FWIW I think a mild sprinkling of unfamiliar things would only add to the excitement, especially because both are very user friendly (and Ollama is free of charge, unlike OpenAI). Anyway, you decide.

Copy link
Member

Choose a reason for hiding this comment

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

Some guide to get this example running would be nice.

Copy link
Author

Choose a reason for hiding this comment

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

oh yes yes, this will be added

import os
import torch
from pathlib import Path
from dotenv import load_dotenv
Copy link
Member

Choose a reason for hiding this comment

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

(no need to fix) FYI there's the FastAI-suggested pydantic-settings, as well as the already-included Starlette Configuration. Such high-level config infras are more popular in real applications.

Comment on lines 1 to 3
Copy link
Member

Choose a reason for hiding this comment

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

(trivial) it's usually a good practice to sort the imports and put them into groups (builtins, libraries, self-package)

Copy link
Member

Choose a reason for hiding this comment

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

(no need to fix) I would leave the images out of version control (like using the GitHub release attachments) and download them during setup.

Comment on lines 4 to 9
Copy link
Member

Choose a reason for hiding this comment

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

(no need to fix) we usually import just the Python modules and use e.g. constants.IMAGES_DIR for easier mocks in tests.

@diksipav diksipav marked this pull request as draft February 24, 2025 09:30
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

Comments