-
Notifications
You must be signed in to change notification settings - Fork 133
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
Adds code for snowflake container example running the UI #1257
Conversation
Note: this is a toy example. For real production needs, you'd need to modify a few things: 1. not use SQLLITE, and instead postgresql, or implement django-snowflake connection. 2. likely not use the Hamilton code within the flask container, instead package up properly and define a UDF or UDTF. 3. could use snowpark dataframes or have hamilton code do other things, e.g. LLM calls..
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.
❌ Changes requested. Reviewed everything up to 7dd1890 in 11 minutes and 18 seconds
More details
- Looked at
324
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. examples/snowflake/hamilton_ui/pipeline_endpoint.py:32
- Draft comment:
TheCHARACTER_NAME
environment variable is defined but not used. Consider removing it if it's not needed. - Reason this comment was not posted:
Confidence changes required:50%
The environment variableCHARACTER_NAME
is defined but never used in the code. This might be a leftover or unnecessary variable.
Workflow ID: wflow_Zye7Oa2UPNLcsRHM
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.
@@ -0,0 +1,14 @@ | |||
FROM python:3.13.1-slim |
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.
Consider using a stable Python version, such as 3.11.x, instead of 3.13.1, which is not a stable release.
tags={"environment": "R&D", "team": "MY_TEAM", "version": "Beta"}, | ||
) | ||
input_columns = { | ||
"signups": pd.Series(spend), |
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.
The spend
and signups
inputs are swapped. It should be:
- name: hamilton-basedir | ||
source: "@<db-name>.<schema-name>.hamilton_base" | ||
$$ | ||
QUERY_WAREHOUSE = <warehause-name> |
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.
There's a typo in warehause-name
. It should be warehouse-name
.
def echo(): | ||
"""This is the endpoint that Snowflake will call to run Hamilton code.""" | ||
message = request.json | ||
logger.debug(f"Received request: {message}") |
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.
Avoid logging sensitive data. The request data being logged here could contain sensitive information. Consider removing or sanitizing this log statement.
# END OF WRAPPER CODE FOR SNOWFLAKE FUNCTION ###### | ||
|
||
|
||
def get_response(prj_id, spend, signups, output_columns): |
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.
The get_response
function is handling multiple responsibilities. Consider refactoring it to adhere to the Single Responsibility Principle by separating the setup, execution, and result processing into distinct functions.
CHARACTER_NAME = os.getenv("CHARACTER_NAME", "I") | ||
|
||
|
||
def get_logger(logger_name): |
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.
The get_logger
function could be reused in other parts of the code. Consider moving it to a separate module or file to adhere to the DRY principle.
Adds code for the post on how to deploy the Hamilton UI on snowflake.
Note: rather than packaging up dependencies that snowflake wants, we use a single container with a flask endpoint to mediate between Hamilton & snowflake. This incurs some overhead but is a quick way to show how to get things to work.
Changes
How I tested this
Notes
It will be related to this post - https://medium.com/@pkantyka/observability-of-python-code-and-application-logic-with-hamilton-ui-on-snowflake-container-services-a26693b46635
Checklist