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

Feature/content to embedding model #18

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

sudan45
Copy link
Collaborator

@sudan45 sudan45 commented Sep 17, 2024

Addresses

Changes

  • Detailed list or prose of changes
  • Breaking changes
  • Changes to configurations

This PR doesn't introduce any:

  • temporary files, auto-generated files or secret keys
  • n+1 queries
  • flake8 issues
  • print
  • typos
  • unwanted comments

This PR contains valid:

  • tests
  • permission checks (tests here too)

@sudan45 sudan45 marked this pull request as draft September 17, 2024 09:02
@sudan45 sudan45 force-pushed the feature/content-to-embedding-model branch from 70ade3e to 65c2884 Compare September 18, 2024 08:40
@sudan45 sudan45 marked this pull request as ready for review September 18, 2024 08:50
.flake8 Outdated
@@ -7,3 +7,4 @@ max-complexity = 10
per-file-ignores =
/**/tests/*_mock_data.py: E501
**/snap_test_*.py: E501
/**/apps.py: F401
Copy link
Member

Choose a reason for hiding this comment

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

Ignore a line not the file itself.

Comment on lines +231 to +236
CELERY_BROKER_URL = "redis://redis:6379/0"
CELERY_RESULT_BACKEND = "redis://redis:6379/0"
CELERY_ACCEPT_CONTENT = ["json"]
CELERY_TASK_SERIALIZER = "json"
CELERY_RESULT_SERIALIZER = "json"
CELERY_TIMEZONE = "UTC"
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Comment on lines 102 to 104
depends_on:
- redis
- web
Copy link
Member

Choose a reason for hiding this comment

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

What?

depends_on:
- db
- redis

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change


volumes:
- .:/code

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

content/tasks.py Outdated
from .models import Content

content = Content.objects.get(id=content_id)
url = settings.EMBEDDING_MODEL_URL
Copy link
Member

Choose a reason for hiding this comment

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

Let's use it directly.


@receiver(post_save, sender=Content)
def content_handler(sender, instance, created, **kwargs):
create_embedding_for_content_task.delay(instance.id)
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to save, but for future use ModelForm (which will then be used in serializers)

content/tasks.py Outdated
for i in range(len(split_docs))
]
if response.status_code == 200:
db = QdrantDatabase(host="qdrant", port=settings.QDRANT_DB_PORT, collection_name=settings.QDRANT_DB_COLLECTION_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

What is this? host="qdrant"

db.store_data(zip(response.json(), metadata))
content.document_status = Content.DocumentStatus.ADDED_TO_VECTOR
else:
content.document_status = Content.DocumentStatus.FAILURE
Copy link
Member

Choose a reason for hiding this comment

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

Where is save?

data = OllamaHandler()
serializer = UserQuerySerializer(data=request.data)
if serializer.is_valid():
result = data.execute_chain(request.data["query"])
Copy link
Member

Choose a reason for hiding this comment

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

Check and add a note if we need async here?

@thenav56 thenav56 self-requested a review September 18, 2024 09:12
@sudan45 sudan45 force-pushed the feature/content-to-embedding-model branch from 89759ba to dcec82c Compare September 27, 2024 04:58
@sudan45 sudan45 force-pushed the feature/content-to-embedding-model branch from dcec82c to dc621cf Compare September 30, 2024 08:56
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