-
Notifications
You must be signed in to change notification settings - Fork 22
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
v1.2.0 update #7
Conversation
Update environment.yml to create the editable install Sproc now returns a table
Set up a virtual environment using [Anaconda](https://conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#creating-an-environment-with-commands) or [virtualenv](https://docs.python.org/3/library/venv.html). | ||
|
||
#### Anaconda | ||
Create and activate a conda environment using [Anaconda](https://conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#creating-an-environment-with-commands): |
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.
I see that before we had instructions for virtualenv wont we support that? Still a lot of users use virtualenv
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.
I'm also torn between the prescriptivism of recommending just conda or also keeping in instructions for virtualenv, since we do have that mentioned in our docs.
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.
For the VS Code integration we went with just conda for simplicity of ensuring environment similarity.
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.
Also torn, my concern was that if someone wants to use a package from PyPi when they deploy to the Snowflake server, the package will come from Conda. So I figured it'll be good to get them on the Conda train sooner so their packages are consistent
src/app.py
Outdated
from snowflake.snowpark.session import Session | ||
from snowflake.snowpark.dataframe import col, DataFrame | ||
from snowflake.snowpark.functions import udf | ||
from src.functions import combine |
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.
wouldn't it be better if we setup the settings in .vscode/settings.json
I typically use:
{
"python.envFile": "${workspaceFolder}/.env",
"files.exclude": {
"**/.git": true,
"Lib": true,
"Include": true,
"Scripts": true,
"**/__pycache__":true
}
}
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.
I don't follow how those settings relate to the imports you highlighted
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.
Sorry let me see if I can explain this better. If you use settings.json file like this:
{
"python.envFile": "${workspaceFolder}/.env",
"terminal.integrated.env.linux": {"PYTHONPATH":"${workspaceFolder}/src:${workspaceFolder}/test:${PYTHONPATH}"},
"terminal.integrated.env.osx": {"PYTHONPATH":"${workspaceFolder}/src:${workspaceFolder}/test:${PYTHONPATH}"},
"terminal.integrated.env.windows": {
"PYTHONPATH": "${workspaceFolder}\\src:.\\test:${PYTHONPATH}"
},
"python.testing.pytestArgs": [
"test"
],
"python.testing.unittestEnabled": false,
"python.testing.pytestEnabled": true,
"python.analysis.extraPaths": [
"${workspaceFolder}/src"
]
}
that configures the tooling to understand that all the code is under ./src/ so you dont have to include src as part of the import which can be confusing. This also works with the debugger and whenever you open a terminal.
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.
If you combine that with some changes in the setup.py like:
from setuptools import setup, find_packages
PACKAGE_NAME = "project1"
setup(
name=PACKAGE_NAME,
version="0.1.0",
# Specify the package directory
package_dir={'': 'src'},
# Include all files from the src directory
package_data={PACKAGE_NAME: ['*']}
)
then you can just do python setup.py bdist_wheel
and you can just renamed that whl to .zip and it will work for deployment
resources.sql
Outdated
@@ -6,17 +6,17 @@ CREATE STAGE IF NOT EXISTS artifacts; | |||
PUT file://&artifact_name @artifacts AUTO_COMPRESS=FALSE OVERWRITE=TRUE; | |||
|
|||
CREATE OR REPLACE PROCEDURE HELLO_WORLD_PROC() | |||
RETURNS integer | |||
RETURNS TABLE(hello_world string) |
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.
This could also be
RETURNS TABLE()
if you think that the simplicity and guiding users to realize they don't need to specify all the columns in the table is nice.
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.
Good catch, I could have sworn when I tried without the type params the SQL would fail. Let me retry
src/app.py
Outdated
|
||
print("Creating session...") | ||
session = Session.builder.configs(get_env_var_config()).create() | ||
session.add_import("src/functions.py", 'src.functions') |
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.
I would recommend using somethign like:
import functions and then
functions.__file__
mostly because that makes the code lets susceptible to path changes. However for that to work well you need to properly setup PYTHONPATH that is why the suggestions in settings.json
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.
Fixed, ran locally fine. Going through PR checks now
@sfc-gh-mrojas -- CI is breaking for some reason, filed an issue here: snowflakedb/snowpark-python#992 |
sys
methods that were used to set the pathStill an open question regarding importing UDFs into sproc