Adding nullable column to postgres images table with type Vector to s…#357
Adding nullable column to postgres images table with type Vector to s…#357ahmednasserswe wants to merge 12 commits intodevelopfrom
Conversation
…tore sscd embedding vectors.
ahmednasserswe
requested review from
DGaffney,
computermacgyver and
skyemeedan
as code owners
|
This looks really promising, probably lots of cool things we can do with direct access to the vector. But it looks like we need to first get Alegre migrated to python 3.8? Build is failing with |
|
Upgrading to 3.8 is a good idea - I'm usually against over-eager version bumps, but this code is now very long in the tooth and 3.8 is very old. @skyemeedan I wonder if we should hold on the version bump until we release all the dependencies off alegre? That would very likely make a bump much easier... |
|
@ahmednasserswe are there earlier versions of pgvector that provide functionality but don't require python 3.8? |
…ions, making sscd vector size 512, and removing version number from pgvector in requirements.tx
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
app/main/model/image.py
Outdated
| raw = remote_response.read() | ||
| im = Image.open(io.BytesIO(raw)).convert('RGB') | ||
| phash = compute_phash_int(im) | ||
| sscd = None |
| from app.main.lib.language_analyzers import init_indices | ||
| from app.main.lib.image_hash import compute_phash_int | ||
| from PIL import Image | ||
| from sqlalchemy import text |
There was a problem hiding this comment.
Imports from package sqlalchemy are not grouped
| from pgvector.sqlalchemy import Vector | ||
|
|
||
| from app.main.lib.presto import Presto | ||
| import json |
There was a problem hiding this comment.
standard import import json should be placed before from flask import current_app as app
| from app.main.lib.image_hash import compute_phash_int, sha256_stream, compute_phash_int, compute_pdq | ||
| from pgvector.sqlalchemy import Vector | ||
|
|
||
| from app.main.lib.presto import Presto |
There was a problem hiding this comment.
Imports from package app are not grouped
…['phash']' to 'post' '/image/similarity/' in test_image_similarity.py'
| # models = ['phash'] | ||
| try: | ||
| models = [m.lower() for m in models] | ||
| except: |
There was a problem hiding this comment.
No exception type(s) specified
| if url: | ||
| image = ImageModel.from_url(url, None) | ||
| result = [] | ||
| image = ImageModel.from_url(url, None,models = models) |
There was a problem hiding this comment.
No space allowed around keyword argument assignment
| if url: | ||
| image = ImageModel.from_url(url, None) | ||
| result = [] | ||
| image = ImageModel.from_url(url, None,models = models) |
There was a problem hiding this comment.
Exactly one space required after comma
|
|
||
| @staticmethod | ||
| def from_url(url, doc_id, context={}, created_at=None): | ||
| def from_url(url, doc_id, context={}, created_at=None, models=None): |
There was a problem hiding this comment.
Too many local variables (18/15)
|
Code Climate has analyzed commit 8b59feb and detected 9 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 41.3% (50% is the threshold). This pull request will bring the total coverage in the repository to 86.4%. View more on Code Climate. |
3 participants
Description
Adding nullable column to postgres images table with type Vector to store sscd embedding vectors.
This included changes to both Postgress dockerfile and Alegre dockerfile.
Reference: TICKET-ID CV2-3917
How has this been tested?
tested locally
Have you considered secure coding practices when writing this code?
No potential security threads