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

Creating endpoint with CRUD for documents and chunks #97

Merged
merged 22 commits into from
Mar 13, 2024

Conversation

bafaurazan
Copy link
Member

@bafaurazan bafaurazan commented Feb 15, 2024

I added file views.py that have declared types of response for client's CRUD request like( GET,POST,PUT,DEL) for chunks and documents, other files exclude .env.example have declared model schemas, url and routing. I think document's response for CRUD is incorect, because i didn't use foreignkey in document's schemas.

Code based on this site: https://medium.com/@hbakri/introducing-django-ninja-crud-ee937bd2ea65

@pgronkievitz
Copy link
Member

@bafaurazan next time - please remember commits are formatted like this

title

some description, remember the empty space above

please, fix the commit message, here's how: https://stackoverflow.com/a/41987851

api/documents/views.py Outdated Show resolved Hide resolved
api/documents/views.py Outdated Show resolved Hide resolved
api/documents/views.py Outdated Show resolved Hide resolved
api/documents/views.py Outdated Show resolved Hide resolved
api/documents/views.py Outdated Show resolved Hide resolved
api/documents/views.py Show resolved Hide resolved
api/documents/schemas.py Outdated Show resolved Hide resolved
api/documents/api.py Outdated Show resolved Hide resolved
@pgronkievitz
Copy link
Member

P.S. when you're ready, remember to select reviewers - I've seen this PR only because I watch all the events in the repo

@bafaurazan bafaurazan force-pushed the Endpoint_for_documents_and_chunks_CRUD branch from 4a277eb to 3ed9e55 Compare February 15, 2024 15:00
I added file views.py that have declared types of response for client's CRUD request like( GET,POST,PUT,DEL) for chunks and documents, other files exclude .env.example have declared model shemas, url and routing. I think document's response for CRUD is incorect, because i didn't use foreignkey.
@bafaurazan bafaurazan force-pushed the Endpoint_for_documents_and_chunks_CRUD branch from 3ed9e55 to dd41b75 Compare February 15, 2024 15:01
I additionally renamed folder containing models for code readability from documents into MyModels
In new version of PUT method, client can update objects declaring in request only variables that he want change (no all variables needed).
Copy link
Member

@pgronkievitz pgronkievitz left a comment

Choose a reason for hiding this comment

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

will CR this properly on saturday/sunday, currently naming is kinda wonky & I think chunks and documents should be separate "django apps"

bafaurazan and others added 4 commits February 23, 2024 21:10
I separated models creating similar chunks folder to documents folder and connect them using routers in api.api file
api/Dockerfile Outdated
Comment on lines 26 to 27
RUN manage.py collectstatic

Copy link
Member

Choose a reason for hiding this comment

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

question: why?

Copy link
Member Author

Choose a reason for hiding this comment

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

i get error message because of this:
image
i'm propably wrong but in my opinion command should looks like: " RUN python manage.py collectstatic", but then i get another error, because (i think) i can't get data contained in .env file
image

so i cant currently use this command and temporarily deleted them

if you can, correct me if i'am wrong

Copy link
Member

Choose a reason for hiding this comment

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

it doesn't work, because

  1. you didn't change workdir in the target stage
  2. you have to use relative path, unlike on windows, linux treats SOME_COMMAND as thing present in PATH and current working directory IS NOT automatically added to path, hence you have to use ./manage.py instead

api/api/api.py Outdated Show resolved Hide resolved
api/api/api.py Outdated Show resolved Hide resolved
Comment on lines 5 to 9
text = models.CharField(max_length=100)
embedding = VectorField(dimensions=10)
chunk_idx = models.IntegerField()
start_char = models.IntegerField()
end_char = models.IntegerField()
Copy link
Member

Choose a reason for hiding this comment

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

issue: please add typing, it looks something like this

text = models.CharField[str, str](max_length=100)

note that you have to add type twice, because it's input/output

api/chunks/schemas.py Outdated Show resolved Hide resolved
api/chunks/views.py Outdated Show resolved Hide resolved
api/chunks/views.py Outdated Show resolved Hide resolved
api/documents/views.py Show resolved Hide resolved
In this commit i add:
- annotations (i don't know if this is correct)
- simple tests (i know that i can't do tests on main database, but i don't know how to do it work with test database (how to add extension with pgvector to test database))
P.S. I type following command to run tests: docker compose exec api python manage.py test --keepdb
- i create files: documents/controllers.py and chunks/controllers.py and defined logic in them that is used by views.py.
@bafaurazan bafaurazan requested review from pgronkievitz and removed request for TheJimmyNowak February 29, 2024 22:19
Copy link
Member

@pgronkievitz pgronkievitz left a comment

Choose a reason for hiding this comment

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

I didn't duplicate issues, so search the rest of the code for similar cases when resolving

Comment on lines 79 to 82
"TEST": {
"NAME": env("POSTGRES_DB"),
},
},
Copy link
Member

Choose a reason for hiding this comment

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

question: What is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

without declaring this lines, tests run by default on the test database: "test_postgres", where pgvector extension is missing. I don't know how to fix this, so i temporarily run tests in the same database (i know this is incorrect), where extension is available.

image

Comment on lines 6 to 10
text: Tuple[str, str] = models.CharField(max_length=100)
embedding: Tuple[list[float], VectorField] = VectorField(dimensions=10)
chunk_idx: Tuple[int, int] = models.IntegerField()
start_char: Tuple[int, int] = models.IntegerField()
end_char: Tuple[int, int] = models.IntegerField()
Copy link
Member

Choose a reason for hiding this comment

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

issue: This is not correct, see my previous comment. It may help you to set pyright to strict

Copy link
Member Author

@bafaurazan bafaurazan Mar 1, 2024

Choose a reason for hiding this comment

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

but your suggestion, didn't work for models, that's why i changed it. The only way i can see is somethinkg like this text: str = models.CharField(max_length=100)

api/chunks/tests.py Outdated Show resolved Hide resolved
api/chunks/tests.py Outdated Show resolved Hide resolved
api/chunks/tests.py Show resolved Hide resolved
api/documents/controllers.py Outdated Show resolved Hide resolved
Comment on lines 6 to 9
class Document(models.Model):
text = models.TextField()
embedding = VectorField(dimensions=10)
chunks = models.ForeignKey(Chunk, on_delete=models.CASCADE, null=True, blank=True)
text: Tuple[str, str] = models.TextField()
embedding: Tuple[list[float], VectorField] = VectorField(dimensions=10)
chunks: Tuple[int, int] = models.ForeignKey(Chunk, on_delete=models.CASCADE, null=True, blank=True)
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect typing

api/documents/views.py Outdated Show resolved Hide resolved
api/documents/views.py Outdated Show resolved Hide resolved
api/documents/views.py Outdated Show resolved Hide resolved
@bafaurazan bafaurazan requested a review from pgronkievitz March 10, 2024 19:02
@bafaurazan
Copy link
Member Author

and Patryk if you could send me some links, that could help me solve the problems with the code, it would help me a lot

@bafaurazan
Copy link
Member Author

should I add pyrightconfig.json file to the project to define how pyright should work?

api/chunks/controllers.py Outdated Show resolved Hide resolved
api/chunks/tests.py Outdated Show resolved Hide resolved
api/chunks/tests.py Outdated Show resolved Hide resolved
api/documents/tests.py Outdated Show resolved Hide resolved
api/documents/tests.py Outdated Show resolved Hide resolved
api/chunks/tests.py Outdated Show resolved Hide resolved
api/documents/tests.py Outdated Show resolved Hide resolved
api/documents/tests.py Outdated Show resolved Hide resolved
api/chunks/tests.py Outdated Show resolved Hide resolved
api/documents/tests.py Outdated Show resolved Hide resolved
@bafaurazan bafaurazan merged commit 45af074 into main Mar 13, 2024
3 checks passed
@bafaurazan bafaurazan deleted the Endpoint_for_documents_and_chunks_CRUD branch March 13, 2024 19:11
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.

2 participants