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

moved to faststream #143

Merged
merged 21 commits into from
Mar 25, 2024
Merged

moved to faststream #143

merged 21 commits into from
Mar 25, 2024

Conversation

gecBurton
Copy link
Collaborator

@gecBurton gecBurton commented Mar 22, 2024

Context

faststream is an abstraction layer for various queuing technologies, by using this instead of pika directly we can:

  • remove the need to handle connections, event etc
  • make use of faststream's in memory queue for testing which makes CI considerably faster as we don't have to install rabbitmq
  • more easily move to another queuing technology like Redis if we need to

Changes proposed in this pull request

  • faststream is installed
  • the ingest and embed workers have been rewritten
  • explicit reference to pika has been removed

Also, the .env was missing from the Dockerfile(s) which caused the e2e tests to fail

Guidance to review

Relevant links

https://technologyprogramme.atlassian.net/browse/REDBOX-99

Things to check

  • I have added any new ENV vars in all deployed environments
  • I have tested any code added or changed
  • I have run integration tests locally

@gecBurton gecBurton marked this pull request as draft March 22, 2024 16:26
@lmwilkigov
Copy link
Collaborator

Oh gosh, this looks fantastic. So clean ✨

@@ -48,7 +48,7 @@ jobs:
mkdir -p data/elastic/
chmod 777 data/elastic/
cp .env.test .env
docker compose up -d --wait elasticsearch minio rabbitmq
docker compose up -d --wait elasticsearch minio
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no need for this in CI anymore

@@ -45,8 +45,6 @@ test-django:
test-integration:
poetry install --no-root --no-ansi --with dev --without ai,streamlit-app,api,django-app,pytest-django,worker,ingest
poetry run pytest tests
docker compose up -d --wait db
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixing an unrelated error

@gecBurton gecBurton marked this pull request as ready for review March 25, 2024 08:46
embed/src/worker.py Outdated Show resolved Hide resolved
ingest/src/worker.py Outdated Show resolved Hide resolved
@@ -1,8 +1,34 @@
import argparse
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

largely brought this back exactly as it was


RUN --mount=type=cache,target=$POETRY_CACHE_DIR poetry install --no-root --no-ansi --with worker,ingest --without ai,dev,api,streamlit-app

FROM python:3.11-slim-buster as runtime
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this contradicts everything above it

from redbox.models.llm import Embedding, EmbeddingResponse

MODEL_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "models")

log = logging.getLogger()
log.setLevel(logging.INFO)

env = Settings()


class SentenceTransformerDB(collections.UserDict):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i have removed the dependancy on the Settings here so that it can be called without an .env existing, e.g. at build time

@lmwilkigov lmwilkigov merged commit 6dd6825 into main Mar 25, 2024
5 checks passed
@lmwilkigov lmwilkigov deleted the feature/faststream branch March 25, 2024 19:48
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.

None yet

2 participants