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

feat: Require login #17

Merged
merged 4 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions app/local.env
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,6 @@ LOG_ENABLE_AUDIT=FALSE
# Change the message length for the human readable formatter
# LOG_HUMAN_READABLE_FORMATTER__MESSAGE_WIDTH=50

############################
# Authentication
############################
# The auth token used by the local endpoints
API_AUTH_TOKEN=LOCAL_AUTH_12345678

Comment on lines -34 to -39
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is leftover from the Flask API code we imported, deleted it to avoid any confusion.

############################
# DB Environment Variables
############################
Expand Down Expand Up @@ -72,4 +66,8 @@ AWS_SECRET_ACCESS_KEY=DO_NOT_SET_HERE

AWS_DEFAULT_REGION=us-east-1

###########################
# DST app configuration
###########################

EMBEDDING_MODEL=/app/models/multi-qa-mpnet-base-dot-v1
1 change: 1 addition & 0 deletions app/src/app_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@

class AppConfig(PydanticBaseEnvConfig):
embedding_model: str = "multi-qa-mpnet-base-dot-v1"
global_password: str | None = None
yoomlam marked this conversation as resolved.
Show resolved Hide resolved
24 changes: 24 additions & 0 deletions app/src/cache.py
KevinJBoyer marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from sentence_transformers import SentenceTransformer

from src.app_config import AppConfig

_app_config: AppConfig | None = None


def get_appconfig() -> AppConfig:
KevinJBoyer marked this conversation as resolved.
Show resolved Hide resolved
global _app_config
Copy link
Contributor

Choose a reason for hiding this comment

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

To facilitate testing, consider minimizing use of global by encapsulating globals in an object. Plus you can use @cached_property (like this) or @property to reduce the boilerplate code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL about the built in cache decorators-- updated the code to use this!


if not _app_config:
_app_config = AppConfig()
return _app_config


_embedding_model: SentenceTransformer | None = None


def get_embedding_model() -> SentenceTransformer:
global _embedding_model

if not _embedding_model:
_embedding_model = SentenceTransformer(get_appconfig().embedding_model)
return _embedding_model
9 changes: 4 additions & 5 deletions app/src/chainlit.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
import logging

from sentence_transformers import SentenceTransformer

import chainlit as cl
import src.adapters.db as db
from src.app_config import AppConfig
from src.cache import get_embedding_model
from src.format import format_guru_cards
from src.generate import generate
from src.login import require_login
from src.retrieve import retrieve

logger = logging.getLogger(__name__)

embedding_model = SentenceTransformer(AppConfig().embedding_model)
require_login()


@cl.on_message
Expand All @@ -21,7 +20,7 @@ async def main(message: cl.Message) -> None:
with db.PostgresDBClient().get_session() as db_session:
chunks = retrieve(
db_session,
embedding_model,
get_embedding_model(),
message.content,
)

Expand Down
4 changes: 2 additions & 2 deletions app/src/ingest_guru_cards.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from smart_open import open

import src.adapters.db as db
from src.app_config import AppConfig
from src.cache import get_embedding_model
from src.db.models.document import Chunk, Document
from src.util.html import get_text_from_html

Expand Down Expand Up @@ -59,7 +59,7 @@ def main() -> None:

logger.info(f"Processing Guru cards at {guru_cards_filepath}")

embedding_model = SentenceTransformer(AppConfig().embedding_model)
embedding_model = get_embedding_model()

with db.PostgresDBClient().get_session() as db_session:
_ingest_cards(db_session, embedding_model, guru_cards_filepath)
Expand Down
14 changes: 14 additions & 0 deletions app/src/login.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import chainlit as cl
from src.cache import get_appconfig


def login_callback(username: str, password: str) -> cl.User | None:
if password == get_appconfig().global_password:
return cl.User(identifier=username)
else:
return None


def require_login() -> None:
if get_appconfig().global_password:
cl.password_auth_callback(login_callback)
32 changes: 32 additions & 0 deletions app/tests/src/test_login.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import os

import chainlit.config
import src.cache
from src.login import login_callback, require_login


def test_require_login_no_password(monkeypatch):
if "GLOBAL_PASSWORD" in os.environ:
monkeypatch.delenv("GLOBAL_PASSWORD")
src.cache._app_config = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear why this line is needed without examining the code. This is probably an artifact of using global variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment to the first instance of this in the file!


require_login()
Copy link
Contributor

Choose a reason for hiding this comment

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

Arguably, this could be replaced with something like app.init(), which would call require_login(). Depends on what you want to demonstrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, ideally I'd like to have some kind of integration test of chainlit.py. Maybe a future improvement?


assert not chainlit.config.code.password_auth_callback


def test_require_login_with_password(monkeypatch):
monkeypatch.setenv("GLOBAL_PASSWORD", "password")
src.cache._app_config = None

require_login()

assert chainlit.config.code.password_auth_callback


def test_login_callback(monkeypatch):
monkeypatch.setenv("GLOBAL_PASSWORD", "correct pass")
src.cache._app_config = None

assert login_callback("some user", "wrong pass") is None
assert login_callback("some user", "correct pass").identifier == "some user"
14 changes: 9 additions & 5 deletions infra/app/app-config/env-config/environment-variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,15 @@ locals {
# }
# }
secrets = {
# Example generated secret
# RANDOM_SECRET = {
# manage_method = "generated"
# secret_store_name = "/${var.app_name}-${var.environment}/random-secret"
# }
CHAINLIT_AUTH_SECRET = {
manage_method = "generated"
secret_store_name = "/${var.app_name}-${var.environment}/CHAINLIT_AUTH_SECRET"
KevinJBoyer marked this conversation as resolved.
Show resolved Hide resolved
}

GLOBAL_PASSWORD = {
manage_method = "manual"
secret_store_name = "/${var.app_name}-${var.environment}/GLOBAL_PASSWORD"
}

OPENAI_API_KEY = {
manage_method = "manual"
Expand Down
Loading