-
Notifications
You must be signed in to change notification settings - Fork 173
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
[ON HOLD] Django settings cleanup, logging improvements and critical fixes #585
base: main
Are you sure you want to change the base?
Conversation
* Improve logging and docs * Fix protobuf generated code version mismtach issue * Add settings/celery.py * Rename celery instance to celery * Refactor celery services config * Rename settings/dev.py to settings/platform.py * Fix env and django settings loading order in backend entrypoints * Remove django settings module update from utils * Remove unnecessary flowerconfig.py * Add env var to suppress evaluation server warnings * Add check for active venv in dev-env-cli.sh
* Allow building docker images locally from current branch * Cleanup run platform script * Update pdm lock files for backend and prompt service
@muhammad-ali-e Observed below error in
Seems like one import was missed in the backend entrypoint for Celery based services after the changes. Please review. |
@tahierhussain Observed that static URL for logo was throwing not found in the login page after the changes: Please review. |
@gaya3-zipstack @kirtimanmishrazipstack Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
from dotenv import load_dotenv | ||
from dotenv import find_dotenv, load_dotenv | ||
|
||
load_dotenv(find_dotenv() or "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hari-kuriakose isn't this the default params / behaviour of the load_dotenv()
?
- Why do we call it this way?
- Isi it necessary to call
load_dotenv()
again?
Quality Gate failedFailed conditions |
missing_settings = [] | ||
|
||
|
||
def get_required_setting( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hari-kuriakose we need to check missing_settings and raise error. Else using the get_required_setting
wouldn't have any effect. Reference: https://github.com/Zipstack/unstract/blob/main/backend/backend/settings/base.py#L532-L536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
) | ||
|
||
from django.core.asgi import get_asgi_application # noqa: E402 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this move to down? Is it for getting envs/settings before loading asgi?
@@ -97,6 +97,8 @@ STRUCTURE_TOOL_IMAGE_TAG="0.0.36" | |||
# Feature Flags | |||
EVALUATION_SERVER_IP=unstract-flipt | |||
EVALUATION_SERVER_PORT=9000 | |||
EVALUATION_SERVER_WARNINGS="false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this wrapped in double quotes. I believe it would still be treated as a string even without the quote
@hari-kuriakose Additionally, we have some tasks (methods/functions) in our backend service that use the Are we planning to separate the consumer from the backend code? |
for more information, see https://pre-commit.ci
Signed-off-by: Hari John Kuriakose <[email protected]>
Quality Gate failedFailed conditions |
|
NOTE: This needs approval from ALL reviewers before merge.
What
backend
versus Celery powered services (execution-consumer
,celery-flower
andcelery-beat
).backend/.env
was not picked up.backend
was breaking when directly running in local due to version mismatch of some protocol buffers generated code. THIS FIXES CONTRIBUTOR INSTALLATIONS.pdm.lock
files forbackend
andprompt-service
.Why
Integrating Celery distributed queue with Django is beneficial but Django settings need to be isolated by component, to ensure only required modules are loaded by Django. For example, various
INSTALLED_APPS
, plugins, etc forbackend
need not be loaded for Celery.settings/dev.py
tosettings/platform.py
might be more apt, as gradually we can keep all common settings inbase.py
, and unique ones inplatform.py
andcelery.py
respectively.settings/celery.py
keeps all Celery related settings in one place and also eliminates the need forflowerconfig.py
..celery.env
ensures env is not polluted.If fetching user orgs failed due to some reason, the error was not logged making it difficult to debug.
DJANGO_SETTINGS_MODULE
should not be set in random places but ONLY in these four main entrypoints, BEFORE importing anything else:backend/asgi.py
(production ASGI)backend/wsgi.py
(production WSGI)manage.py
(development server)backend/celery.py
(Celery powered services)Protocol buffers by nature generates some stub code for client and server integration from a
.proto
file. However, this stub code needs to be regenerated should the protocol buffers dependency version change. The easier alternative is to havePROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python
env setting which lets Python internally handle the stub code generation although relatively slower. This is what we do. However the other issue where the env vars and Django settings got loaded out of order caused mentioned protocol buffers env var to be unset, thus breakingbackend
.Contributors should be able to run services directly in local always.
Evaluation server if not running will throw warnings, which could be suppressed by default to remove noise from the logs. We could have an option to show warnings when required, which will continue to help in debugging.
A local build of Docker images should be allowed to happen from any feature branch instead of
main
only.Executing
dev-env-cli.sh
while another virtual env is already activated will cause undesired effects, hence need to be checked.pdm.lock
files base branch caused resolution errors, hence needed to be regenerated manually.How
Features
settings/celery.py
.celery.env
and corresponding.sample.celery.env
backend/celery.py
entrypoint forexecution-consumer
,celery-flower
andcelery-beat
celery
to avoid confusionsettings/dev.py
tosettings/platform.py
flowerconfig.py
dev-env-cli.sh
run-platform.sh
Fixes
backend
entrypoints.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
backend
(v1 and v2),execution-consumer
,celery-flower
andcelery-beat
.Database Migrations
Env Config
EVALUATION_SERVER_WARNINGS
(default'false'
) inbackend/.env
.DJANGO_SETTINGS_MODULE='backend.settings.platform'
.backend/.celery.env
.Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
The following modes of invocations seem to be running fine:
cd backend && . .venv/bin/activate
)./entrypoint.sh
python manage.py runserver 0.0.0.0:8000
.venv/bin/celery -A backend worker --loglevel=info -Q celery,celery_periodic_logs,celery_log_task_queue --autoscale=8,1
.venv/bin/celery -A backend beat --scheduler django_celery_beat.schedulers:DatabaseScheduler -l INFO
.venv/bin/celery -A backend flower --port=5555 --purge_offline_workers=5
./run-platform.sh -u -b -v current
Screenshots
Checklist
I have read and understood the Contribution Guidelines.