Repository navigation
Refactor and improve configuration handling across the project - #23
Merged
Merged
Conversation
The suite had no way to run: the repo referenced a DJANGO_SETTINGS_MODULE pytest option that was never defined, and the Django test modules (chat, data, engine, authorization) all fail at import with ImproperlyConfigured. Introduce sse_rest_api/tests/settings_django.py: an in-memory SQLite configuration with every app installed, Keycloak as the active auth provider (authorization.core.handlers raises at import unless one is), and a SYSTEM_HANDLER stub. Wire it up via [tool.pytest.ini_options] so a plain 'python -m pytest' from the repo root collects and runs the whole suite. Also implement the contract tests in tests/test_env_utils.py were written against: bool_config_value, split_list_value and list_env_values were missing from main/src/env_utils, so importing the test module failed outright.
authorization/models.py defines SessionState and Token, but authorization/migrations/ was empty, so 'manage.py migrate' never created the authorization_token and authorization_sessionstate tables. The introspection code path in authorization.core.handlers raises 'no such table' at runtime. Generate 0001_initial.py. Existing deployments that already created the tables manually should run 'python manage.py migrate authorization --fake 0001_initial' (or --fake-initial) to align state.
tests/test_config_examples.py and engine/tests/test_similarity.py reference config templates that did not exist, so they failed at collection. Create the five *.example.* files the guard tests pin: django-config.example.json (production-safe), milvus_config.example.json, auth-config.example.json, user-group-organisation.example.json and secret-key.example.txt. Every secret carries a CHANGE_ME placeholder so a template can never leak a real credential. .gitignore now explicitly un-ignores the *.example.* templates; the real configs (django-config.json, secret-key.txt, ...) stay untracked.
The repository carried three different source roots and relied on ad-hoc
sys.path bootstrapping: the Django apps were imported as top-level
modules (chat, data, engine, system, main), sse_apps reached them by
inserting sse_rest_api into sys.path, and scripts/ copied Python files
next to itself before running them.
New layout:
src/sse_api/ Django project (was sse_rest_api/)
config/ settings, urls, wsgi, asgi, celery (was main/)
core/ shared utilities (was main/src/)
chat|data|engine|system|authorization|content_supervisor/
src/sse_tools/ CLI tools (was sse_apps/)
configs/ runtime configuration, was sse_rest_api/configs/
scripts/ thin entry points, no more copy-and-run
The project is now installed with 'pip install -e .' so every module is
importable from anywhere and the sys.path hacks are gone.
Import safety:
- every Django AppConfig pins label = <app>, so app_label, migration
dependencies, content types and existing database tables are unchanged;
- migration files and cross-app model references keep the bare label;
- authorization/__init__ keeps app_name as the module path used for the
middleware and authentication class dotted paths.
Runtime configuration is now located through SSE_CONFIG_DIR (default
'configs'), replacing the implicit assumption that the process starts in
sse_rest_api/. Dockerfile, docker-compose.yml, run.sh, run-en.sh,
initialize.sh, run-api*.sh and gunicorn.conf.py all follow.
Also fixes configs/generative-models.json, which was not valid JSON
(trailing commas) and therefore crashed every import of the generative
model controller.
Co-Authored-By: Codex <noreply@openai.com>
engine/ was the only app carrying two layer packages at once: core/ (errors, middleware) and engine_core/ (protocols, search engine, query pipeline, config, generative). The doubled name added nothing and made import paths needlessly long. Move every engine_core module into engine.core and re-point the nine modules that imported it. engine/ now follows the same shape as the other apps: api.py, controllers/, core/, models.py, urls.py (plus a database/ adapter layer). No behaviour change; the full suite still passes.
The client library referenced the old server module paths (main.src.*, sse_rest_api/) in its docstrings, README and the endpoint-contract test that reads the Django route files. Re-point them to sse_api.* and src/sse_api/ so the contract test still validates the library against the backend routes. No behaviour change; the sse_lib suite still passes (104 tests).
Lint configuration was spread across four files with overlapping coverage: [tool.black] and [tool.bandit] in pyproject.toml, [flake8] in tox.ini, [bandit] in .bandit, [mypy] in mypy.ini and a 639-line .pylintrc that also carried a [MESSAGES CONTROL] section. Consolidate everything that has a TOML home into pyproject.toml (black, bandit, pylint, mypy). flake8 cannot read TOML, so its options move to setup.cfg. The redundant tox.ini and mypy.ini are deleted. check-dev.sh now takes a mode argument (format / lint / types / test) instead of running everything with no way to narrow it down, and bandit is scoped to src/ and scripts/ so it does not scan the vendored docs. Add the CI workflow that .github/workflows/ was pointing at but never contained: a 3.10/3.11/3.12 test matrix plus a lint job running the same checks check-dev.sh runs, so local and CI agree on the verdict.
README.md still documented the pre-refactor structure (apps_sse/, main/, semantic_search_engine/, flat app packages), and SETUP.md, DEPENDENCIES.md, EVALUATE.md, sse_lib/README.md, .env.example and the per-app READMEs all pointed at sse_rest_api/ paths, main.src modules and a bare manage.py. Rewrite the README directory layout to match src/sse_api, src/sse_tools and configs/, and re-point every remaining reference: sse_rest_api/ -> src/sse_api/ sse_rest_api/configs/ -> configs/ main.src.* -> sse_api.core.* main.settings -> sse_api.config.settings manage.py <cmd> -> python -m sse_api.manage <cmd> apps_sse/* -> src/sse_tools/* No stale references to the old layout remain anywhere in the repo.
- fix NameError in chat/controllers/message.py: add missing logging import - fix NameError in engine/controllers/models_logic/generative.py: percentage_rank_gen_qa -> percentage_rank_mass - fix F821 in config/settings.py: import authorization properly - remove unused imports (F401), reflow long imports (E501/E128) - fix whitespace (E302/E303/E306) and star-import noqa placement - fix F541 f-string without placeholders in tests_loader.py - flake8 config: max-line-length 100, per-file-ignores for re-exports
- .bandit was written in INI style, but bandit >= 1.7 parses it as a YAML
profile ('key: value' is not valid YAML), so every bandit run - locally
via check-dev.sh and in CI - aborted with a parse error. Rewrite it as a
YAML profile; excluded dirs now come from the command line.
- Remove the dead [tool.bandit] section from pyproject.toml (bandit has
never read TOML) and drop strict=true from [tool.mypy]: the codebase is
untyped and strict mode reports 1500+ errors, so the mypy CI job could
never pass. Non-strict mypy stays the gate (opt-in locally, advisory in
CI).
- flake8 scope: exclude sse_lib (standalone package with its own setup)
like the original tox.ini did, instead of scanning it with the app's
settings.
- mypy target src/sse_lib does not exist; the package lives at sse_lib/.
Fix it in both CI and check-dev.sh.
- .pylintrc was a 600-line dump of every default option; keep only the
deviations (py-version, message control, line length).
black --check . now passes (32 files reformatted). The flake8 gate covers the whole repo (except sse_lib), so the remaining pre-existing E501 lines are wrapped: Polish prompt strings in message.py and evaluate_rag_llm.py, date-filter expressions in template.py, a long assertion in test_input_validation.py, docstring/comment lines in protocols.py and vector_store_adapter.py, and a commented-out sample in evaluate_text_to_human.py. Docs: point EVALUATE.md / SETUP.md / DEPENDENCIES.md / README.md at the new src/ paths (the sse_apps/not_tested and main/ references were stale after the layout refactor).
`sse_api.tests.settings_django` defined no ROOT_URLCONF, so every test had to call a view directly through APIRequestFactory. That made a whole class of defect invisible: `GET /api/healthz`, polled by nginx and by the container HEALTHCHECK, shipped without ever being registered and nothing noticed. Inert for the tests that exist today — Django loads the URLconf lazily on the first request, and no current test makes one.
The Dockerfile HEALTHCHECK curls /api/healthz, nginx/sse.conf routes that path
to the api upstream and docker-compose gates services on service_healthy, yet
no urls.py registered it. The probe answers 404, `curl -f` fails on it, so
every container reports unhealthy from the first deploy. The dependency checks
were already written in system/core/health.py — they just had no caller.
SSEHealthCheck exposes them: public, because a probe carries no token, and
200/503 so that `curl -f` and service_healthy react to it. It is registered
both under the configured API root and as a bare /healthz, so turning on
api.major_version (which makes the root api/v1) cannot silence health reporting
in silence. An exception raised inside a check yields 503 instead of a 500 with
a traceback.
The body is {"healthy": bool, "checks": {...}} rather than the usual envelope
key `status`: sse_lib detects the envelope by `payload["status"] is True`, so a
probe replying {"status": "ok"} would be read as a failed API call by its own
client on a perfectly healthy server.
sse_lib gains health() and is_healthy() so a deployment can ask the same
question in code. The server route and the client-side endpoint constant land
in one commit because test_contract.py asserts the two route sets are equal in
both directions; either half alone leaves the suite red.
Adds 8 server tests and 10 client tests. Suite: 348 passed, 144 subtests;
flake8 and black clean.
RdlAuthStateHandler and RdlAuthGrantAccTokenHandler open configs/auth-config.json from their constructors, and the views held them as class attributes. Importing sse_api.authorization.urls therefore raised FileNotFoundError on any machine without a deployment config file, which is exactly when Django does it — while loading the URLconf. AuthAuthenticationMiddleware had the same defect through GATokenAuthentication(). Views now resolve their handler via cached_property (DRF builds the view per request, so no handler is shared across threads any more) and the middleware builds its collaborator in __init__, where Django already guarantees settings are configured and still only once per process. test_endpoint_input.py patched RdlAuthConfig.load_config purely to be able to import the views; that scaffolding is gone and the provider is patched on its class instead of on the instance the view used to expose. Verified: authorization.urls and authorization.core.middleware now import with no config file present. Suite 348 passed, 144 subtests; flake8 and black clean.
config.urls assembles a different route set per authentication mode (token, Keycloak, OAuth v1, OAuth v2) and nothing exercised that switch — each branch was only reachable by hand-editing a deployment config file. With all four modes in scope for the production release, they need tests. Characterization tests: they pin what each mode produces today. Writing them turned up a defect worth recording, so it is pinned explicitly rather than fixed here: in every external-auth mode api/login is registered twice — once by system/urls.py for username/password, once by authorization/urls.py for the OAuth code exchange — and config.urls appends the authorization routes before the system ones, so Django resolves the code-exchange view and OrganisationLogin is unreachable. sse_lib.login() still posts credentials to that path. The test documents the collision and says to update it when fixed. Possible now that authorization.urls no longer reads config at import time. Suite: 357 passed, 157 subtests; stable in both run orders; flake8, black clean.
The server validates both keys of every text document with required_text (core/validation.py: texts -> filepath, relative_filepath), so the default text_document(relative_path="") made every index_documents() call invalid before it was even sent. An omitted path now falls back to the document name. One test asserted the old shape and was pinning a payload the server rejects; updated, plus a test that the field is never blank. The class docstring example gained the percentage_rank_mass the server requires in query_options, so the documented quickstart no longer teaches a request that is guaranteed to fail validation, and the transport= argument now says plainly that api_prefix/timeout/verify/session/headers/max_retries are ignored when a transport is handed in rather than silently dropped. Suite: 358 passed, 160 subtests; flake8 and black clean.
max_tokens_in_chunk and number_of_overlap_tokens go through identifier()/ number() in the data app, and percentage_rank_mass through generation_options() in the engine; all three reject None. The option builders sent None, so every index_documents()/upload_files() call without explicit options, and the documented generate_answer() example, failed server-side validation with an error that named none of the fields. The client now checks the same rules before it sends anything and raises SSEValueError naming the missing field. Validation is applied per endpoint, not per option class, because the two paths disagree on purpose: generative_answer validates query_options with generation_options while add_user_message accepts any object, so a class-level rule would reject chat requests that work today. A test pins that from the other side. No defaults are invented. Chunk size follows the context window of whichever embedder a deployment is configured with, which a client library cannot know, so callers pass it explicitly or set default_indexing_options once; both READMEs now show the call that actually works instead of one that cannot. 9 tests that asserted the rejected payloads were updated, 13 added covering the new errors and the non-over-constrained chat path. Suite: 370 passed, 162 subtests; flake8 and black clean.
…n tags, and set `PYTHONPATH` in scripts
- Automatically activate virtual environments (`.venv` or `venv`) in all relevant scripts. - Introduce reusable `docker_cmd` wrapper to handle scenarios where `sudo` may be required for Docker commands. - Enhance Python and pip discovery by adding logic to prioritize virtual environment paths. - Update scripts to ensure configuration is reusable across multiple environments and to adapt to deployment scenarios without manual intervention.
…cripts to include new `config` command and `bs4` dependency
…cripts to include new `config` command and `bs4` dependency
…ability, set default `DJANGO_SETTINGS_MODULE`, and enhance URI schema handling across scripts and modules
…ved accessibility and update allowed hosts in configuration
…rect environment and JSON config handling - Simplify settings retrieval by directly checking environment variables and JSON configuration. - Remove the unnecessary use of `_get_configuration_for_variable` in favor of explicit methods. - Enhance code readability and maintainability by consolidating configuration parsing logic.
…t parsing and configuration handling
…roved readability and hierarchy
…support - Introduce test cases for validating default and custom index parameters in `MilvusHandler`. - Ensure proper invocation of Milvus client methods for index creation and collection handling. - Enhance test readability with structured assertions and mock-based isolation.
…ndex metadata - Update `_FakeMilvusClient` and `MilvusHandler` to handle metric types dynamically. - Add helper method `__prepare_search_params` for extracting metric type from existing indexes. - Introduce and enhance unit tests to validate dynamic metric type behavior for searches.
…dize URL handling - Add `_create_router_client` to encapsulate client creation logic with support for token authentication. - Standardize URL formatting across active model and API configurations. - Update environmental variables and documentation to reflect new token support.
…ayload handling and router client behavior - Add comprehensive test cases to validate router client creation, typed payload processing, and environmental variable configuration. - Ensure proper handling of model configs, scheme-less hosts, and conversation workflows.
pawkedzia
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of Changes