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: add docker and nginx support #388

Merged
merged 3 commits into from
Oct 13, 2024
Merged

Conversation

97k
Copy link
Contributor

@97k 97k commented Oct 11, 2024

[Short description explaining the high-level reason for the pull request]
api.localhost will direct to api on port 7242 and telemetry.localhost will open the Burr UI

Changes

  • add Docker support along with compose file
  • nginx as a proxy for api and burr UI
  • bug fixes

How I tested this

Tested the docker build

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Important

Adds Docker and Nginx support for email assistant with proxy configuration and minor server updates.

  • Docker and Nginx Support:
    • Adds Dockerfile to set up a Python environment and run wrapper.sh.
    • Adds docker-compose.yaml to define services for the app and Nginx, exposing ports 7241 and 7242.
    • Adds nginx.conf to proxy api.localhost to port 7242 and telemetry.localhost to port 7241.
  • FastAPI Server:
    • Adds root endpoint in server.py to return a welcome message.
    • Includes router with prefix /email_assistant in server.py.
  • Miscellaneous:
    • Fixes type hint bug in _run_through() in server.py.
    • Adds wrapper.sh to start Burr UI and FastAPI server.

This description was created by Ellipsis for 54a7735. It will automatically update as commits are pushed.

api.localhost will direct to api on port 7242 and telemetry.localhost will open the Burr UI
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 54a7735 in 22 seconds

More details
  • Looked at 144 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. examples/email-assistant/Dockerfile:9
  • Draft comment:
    Consider adding a .dockerignore file to exclude unnecessary files from the Docker build context, which can speed up the build process and reduce the image size.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The Dockerfile is missing a COPY command for the requirements.txt file before the RUN pip install command. This can lead to caching issues during the build process.
2. examples/email-assistant/docker-compose.yaml:25
  • Draft comment:
    Consider removing the name field under networks to avoid potential conflicts with network names when running multiple instances.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The docker-compose file is using a fixed network name which might cause conflicts if multiple instances are run. It's better to let Docker Compose handle network naming.
3. examples/email-assistant/nginx.conf:7
  • Draft comment:
    Ensure that your local DNS or hosts file is configured to resolve telemetry.localhost and api.localhost to the correct IP address.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The nginx configuration is set to listen on port 80 for both servers. This is correct as per the PR description, which mentions api.localhost and telemetry.localhost. However, ensure that the DNS or hosts file is configured to resolve these domains correctly.
4. examples/email-assistant/server.py:189
  • Draft comment:
    Ensure that moving app.include_router outside the if __name__ == "__main__": block is intentional and does not affect other parts of the application.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The app.include_router line was moved outside the if __name__ == "__main__": block, which is correct. However, ensure that this change is intentional and does not affect other parts of the application.

Workflow ID: wflow_nPGAsQMLNy1n3EVW


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Look great. Everything works. We just need to add to the README on how to run this (maybe even also put this as a comment in the docker file itself).

E.g. to run the UI, along with the email server backed, in a docker container:

  1. docker build . -t email_assistant
  2. docker run -it -p 7241:7241 -p 7242:7242 -e OPENAI_API_KEY='...' email-assistant
  3. Navigate to localhost:7242/docs for the email server running burr
  4. Navigate to localhost:7241 for the Burr UI
  5. Go to demos and play with the email-assistant if you don't have a UI. Else connect to the burr email server with your UI code.
    Notes: this does not mount a persistent volume, so state is lost once the container goes down.

Any other caveats?

@97k
Copy link
Contributor Author

97k commented Oct 12, 2024

@skrawcz Yes,
in fact
docker compose up --build is all that's required.

I will add a Readme file and yes there's no persistance.

And with compose file we can

  • access BURR UI on telemetry.localhost
  • access API on api.localhost

@97k
Copy link
Contributor Author

97k commented Oct 13, 2024

@skrawcz Let me know if any changes are required.

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

:shipit:

@skrawcz skrawcz merged commit a1a0b3b into DAGWorks-Inc:main Oct 13, 2024
10 of 11 checks passed
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.

2 participants