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: Reload support for backend, changes to entrypoint script #1100

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

chandrasekharan-zipstack
Copy link
Contributor

@chandrasekharan-zipstack chandrasekharan-zipstack commented Jan 28, 2025

What

  • Added options for reload and graceful timeout to improve dev experience for backend
  • Added support to override docker compose config to help run services with less memory
  • entrypoint.sh changes to perform migration and run in dev mode
  • Added inotify dev dependency to perform better file watching with fewer resources

Why

  • Reload option helps with looking for file changes and performs a hot reload of the django server helping the dev experience

How

  • Added as a parameter to the script. Can be used with pdm run backend --dev

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)

  • No, only changes to improve dev experience

Relevant Docs

Dependencies Versions

  • inotify>=0.2.10

Notes on Testing

  • Tried making changes locally and tested the reload feature

Screenshots

image

  • Reload behaviour
    image

Checklist

I have read and understood the Contribution Guidelines.

Comment on lines 27 to 30
start_time = time.time()
django_app = get_wsgi_application()
logger.info(f"WSGI application initialized in {(time.time() - start_time):.3f} seconds")

Copy link
Contributor

Choose a reason for hiding this comment

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

May be use time.perf_counter() . This might be more accurate for these sort of use case

Copy link
Contributor

@ritwik-g ritwik-g left a comment

Choose a reason for hiding this comment

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

Great initiative solving real pain point for the developers. The changes looks good. Let's keep improving small bits in similar fashion

@@ -636,6 +636,7 @@ docker/*.env
!docker/sample*.env
docker/public_tools.json
docker/proxy_overrides.yaml
docker/compose.override.yaml
Copy link
Contributor

@hari-kuriakose hari-kuriakose Jan 31, 2025

Choose a reason for hiding this comment

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

@chandrasekharan-zipstack This might not be needed.

We can check in the actual overrides yaml themselves. It will be activated only when you pass them via the -f flag, hence mentioned.

Copy link
Contributor

filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{9}}$$ $$\textcolor{#23d18b}{\tt{9}}$$

Copy link
Contributor

@hari-kuriakose hari-kuriakose left a comment

Choose a reason for hiding this comment

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

@chandrasekharan-zipstack LGTM overall.

Extending this we can have docker/compose_overrides/worker.yaml etc and check them in. These will become built-in templates which can be picked by the devs and used as desired. See #1100 (comment) too.

Overall great initiative and really happy to see this. Let's keep improving one small thing at a time. All the best!

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.

3 participants