-
Notifications
You must be signed in to change notification settings - Fork 110
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
SNOW-869906: Tag package auto-upload feature as experimental #947
SNOW-869906: Tag package auto-upload feature as experimental #947
Conversation
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 parameters added in existing functions like add_packages
, can you also update the docstrings to mention the parts that will be experimental
…NOW-869906-experimental-tag # Conflicts: # src/snowflake/snowpark/session.py
Codecov Report
@@ Coverage Diff @@
## vnayak-SNOW-849031-environment-persistence #947 +/- ##
===========================================================================
Coverage 98.55% 98.55%
===========================================================================
Files 50 50
Lines 9024 9026 +2
Branches 1613 1613
===========================================================================
+ Hits 8894 8896 +2
Misses 53 53
Partials 77 77
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/snowflake/snowpark/session.py
Outdated
|
||
Note that this function also supports addition of requirements via a `conda environment yaml file | ||
<https://conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#create-env-file-manually>_`. | ||
|
||
|
||
Args: | ||
file_path: The path of a local requirement file. | ||
force_push: Force upload Python packages with native dependencies. | ||
force_install: Ignores environment present on persist_path and overwrites it with a fresh installation. | ||
force_push: Force upload Python packages with native dependencies (experimental). |
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 suggest add a *
after file_path
to make the 3 new arguments key-word arguments.
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.
Added
@@ -727,10 +728,11 @@ def add_packages( | |||
:class:`~snowflake.snowpark.udf.UDFRegistration`. See details of | |||
`third-party Python packages in Snowflake <https://docs.snowflake.com/en/developer-guide/udf/python/udf-python-packages.html>`_. | |||
|
|||
Pure Python packages that are not available in Snowflake will be pip installed locally and made available as an | |||
EXPERIMENTAL: Pure Python packages that are not available in Snowflake will be pip installed locally and made available as an |
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.
Better explain how this experimental feature is enabled. For instance, xyz argument is set to True, then the packages will be automatically installed from pip.
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 feature is enabled when a custom package not present on Anaconda channel is passed to add_requirements
and add_packages
. Do you want me to only allow the feature via a specific argument instead?
src/snowflake/snowpark/session.py
Outdated
@@ -742,10 +744,10 @@ def add_packages( | |||
is supported as a `version specifier <https://packaging.python.org/en/latest/glossary/#term-Version-Specifier>`_ | |||
for this argument. If a ``module`` object is provided, the package will be | |||
installed with the version in the local environment. | |||
force_push: Force upload unavailable Python packages with native dependencies. | |||
force_install: Ignores environment present on persist_path and overwrites it with a fresh installation. | |||
force_push: Force upload unavailable Python packages with native dependencies (experimental). |
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.
"native" dependency might be confusing. People may think of native c code. Do you mean "local"?
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.
No I mean native C code, will rewrite this.
Moved these changes to #959 so that they can be directly merged to main (This PR was stacked on top of other changes). |
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.