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

[Doc] Update Vizro docs and example template #306

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Jan 16, 2025

Thanks for adding vizro in #298! I just have a few corrections and simplifications.


📚 Documentation preview 📚: https://ploomber-doc--306.org.readthedocs.build/en/306/

dashboard = vm.Dashboard(pages=[page])

# Create the application instance
app = Vizro().build(dashboard)

# Expose the Flask server to Gunicorn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

N.B. this is a small simplification Vizro offers over Dash/Flask - no need to explicitly expose the underlying server. The app object already acts as the WSGI server.


# Copy application files
COPY . .

# Configure the container
EXPOSE 80
ENTRYPOINT ["gunicorn", "app:server", "run", "--bind", "0.0.0.0:80"]
ENTRYPOINT ["gunicorn", "app:app", "--bind", "0.0.0.0:80"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

run doesn't seem to exist unless I missed something?

Copy link
Member

Choose a reason for hiding this comment

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

run doesn't exist. Turn out all our Dockerfile template have it, and the gunicorn ignore it. Thanks for point that out

examples/docker/vizro/Dockerfile Outdated Show resolved Hide resolved
@@ -1,24 +1,14 @@
FROM python:3.11-slim
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 now matches the docs file exactly.

@@ -1,24 +1,14 @@
FROM python:3.11-slim

# Set environment 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.

As far as I could tell these weren't actually necessary for anything. e.g. PIP_NO_CACHE_DIR=off is overridden below anyway by --no-cache-dir.

COPY . .

# Expose the port
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure this should have been 80 all along?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's my error, the port 80 is also is exposed by the platform, so it didn't notice it

examples/docker/vizro/requirements.txt Outdated Show resolved Hide resolved
@edublancas
Copy link
Contributor

@LatentDream please review

Copy link
Member

@LatentDream LatentDream left a comment

Choose a reason for hiding this comment

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

Tested ✅

@edublancas edublancas merged commit b7b4080 into ploomber:main Jan 17, 2025
1 check passed
@edublancas
Copy link
Contributor

thanks, @antonymilne

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