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

Run streamlit dashboard on docker container #1571

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

Conversation

sfc-gh-pdharmana
Copy link
Contributor

@sfc-gh-pdharmana sfc-gh-pdharmana commented Oct 18, 2024

Description

Please include a summary of the changes and the related issue that can be
included in the release announcement. Please also include relevant motivation
and context.

Other details good to know for developers

Please include any other details of this change useful for TruLens developers.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • New Tests
  • This change includes re-generated golden test results
  • This change requires a documentation update

Important

Add Docker setup and Snowflake integration for Streamlit dashboard with spcs_runtime support.

  • Docker Setup:
    • Adds Dockerfile to build a container for Streamlit dashboard.
    • Adds requirements.txt for dependencies.
    • Adds run_container.py and run_dashboard.py for configuring and running the dashboard.
  • Code Changes:
    • Adds host parameter to SnowflakeConnector in connector.py.
    • Adds spcs_runtime option to run_dashboard() in run.py.
    • Updates init_from_args() in streamlit.py to handle spcs_runtime and Snowflake token-based authentication.

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

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Oct 18, 2024
Copy link
Contributor

@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.

❌ Changes requested. Reviewed everything up to d853b10 in 32 seconds

More details
  • Looked at 357 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/dashboard/trulens/dashboard/streamlit_utils.py:61
  • Draft comment:
    Storing passwords directly in the code is a security risk. Consider using environment variables or a secure vault to manage sensitive information.
  • Reason this comment was not posted:
    Marked as duplicate.
2. tools/snowflake/spcs_dashboard/Dockerfile:8
  • Draft comment:
    Combine the pip install commands into a single RUN statement to reduce the number of layers in the Docker image and improve build performance.
RUN pip install -r requirements.txt \
    && pip install trulens_connectors_snowflake-1.0.1-py3-none-any.whl \
    && pip install trulens_dashboard-1.0.1-py3-none-any.whl
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The Dockerfile should be optimized to reduce the number of layers and improve build performance.

Workflow ID: wflow_OnhmyXvTHxR6XD30


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

db_url = URL(
account=snowpark_session.get_current_account(),
user=snowpark_session.get_current_user(),
password="password",
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing passwords directly in the code is a security risk. Consider using environment variables or a secure vault to manage sensitive information.

session = Session.builder.configs(connection_parameters).create()

# Create compute pool if it does not exist
compute_pool_name = input("Enter compute pool name: ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Using input() for getting the compute pool name is not suitable for automated scripts or production environments. Consider using environment variables or command-line arguments instead.

Copy link
Contributor

@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! Incremental review on 272a194 in 13 seconds

More details
  • Looked at 33 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/dashboard/trulens/dashboard/streamlit.py:68
  • Draft comment:
    The import statement for os is repeated. Consider moving it to the top with other imports to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for 'os' is repeated within the 'if args.spcs_runtime:' block. It should be moved to the top with other imports to avoid redundancy.
2. src/dashboard/trulens/dashboard/streamlit.py:40
  • Draft comment:
    Unnecessary blank line after function definition. Remove for better readability. This applies to line 47 as well.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    There is an unnecessary blank line after the function definitions get_spcs_login_token and init_from_args. These should be removed for better code readability.

Workflow ID: wflow_LdspBlL0Ine8c6an


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

Comment on lines +9 to +10
RUN pip install trulens_connectors_snowflake-1.0.1-py3-none-any.whl
RUN pip install trulens_dashboard-1.0.1-py3-none-any.whl
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 is temporary, once the next version of trulens is released, then this can go in requirments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check with corey whether requirements.txt can be done using poetry instead.

@sfc-gh-pmardziel
Copy link
Contributor

I'm not familiar enough with the snowflake setup to check most of this PR. One generic comment about the python: if possible, can anything snowflake related be separated into different files?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants