Snow 1260461 refactor snowpark build #937
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| ignore_anaconda=ignore_anaconda, | ||
| skip_version_check=skip_version_check, | ||
| pip_index_url=index_url, | ||
| allow_shared_libraries=allow_shared_libraries_yesnoask, |
There was a problem hiding this comment.
It seems that native libs checks happens later on, why download requires the argument?
There was a problem hiding this comment.
Propably a leftover after merge - nice catch!
Comment on lines
355
to
360
| allow_shared_libraries_yesnoask = { | ||
| True: YesNoAsk.YES, | ||
| False: YesNoAsk.NO, | ||
| }[allow_shared_libraries] | ||
| if deprecated_package_native_libraries != YesNoAsk.NO: | ||
| allow_shared_libraries_yesnoask = deprecated_package_native_libraries |
There was a problem hiding this comment.
Let's try to remove this piece of code by using or in if
if package_utils.detect_and_log_shared_libraries(
download_result.downloaded_packages_details
)
if not (allow_shared_libraries or resolve_allow_shared_libraries_yes_no_ask(...)):
raise ClickException(
"Some packages contain shared (.so/.dll) libraries. "
"Try again with --allow-shared-libraries."
)WDYT?
There was a problem hiding this comment.
Makes sense - I decided to merge them as they were passed as a single argument to the download_unavailable_packages function, but that's not a case anymore
Comment on lines
202
to
215
| # The package is not in anaconda, so we have to pack it | ||
| log.info("Checking to see if packages have shared (.so/.dll) libraries...") | ||
| if detect_and_log_shared_libraries(download_result.downloaded_packages_details): | ||
| if not resolve_allow_shared_libraries_yes_no_ask( | ||
| allow_shared_libraries_yesnoask | ||
| ): | ||
| raise ClickException( | ||
| "Some packages contain shared (.so/.dll) libraries. " | ||
| "Try again with --allow-shared-libraries." | ||
| ) | ||
|
|
||
| zip_file = f"{get_package_name(name)}.zip" | ||
| zip_dir(dest_zip=Path(zip_file), source=packages_dir.path) | ||
| message = dedent( |
There was a problem hiding this comment.
As discussed in the other thread we should split download from checking for native libraires. In this way code is easier to compose and test.
There was a problem hiding this comment.
This is already a separate part of command code (or I don't see something :D)
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | Generic Password | 41d3f90 | tests/test_connection.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
The above secret(s) have been detected in your PR. Please take an appropriate action for each secret:
- If it’s a true positive, remove the secret from source code, revoke it and migrate to a secure way of storing and accessing secrets (see http://go/secrets-and-code). Once that’s done, go to the incidents page linked in the “GitGuardian id” column (log in using SnowBiz Okta) and resolve the incident.
- If it’s a false positive, go to the incidents page linked in the “GitGuardian id” column (log in using SnowBiz Okta) and ignore the incident.
- If you didn’t add this secret (and only then), you can skip this check manually.
Note:
- A secret is considered leaked from the moment it touches GitHub. Rewriting git history by force pushing or other means is not necessary and doesn’t change the fact that the secret has to be revoked.
- This check has a “Skip: false positive” button. Don’t use it. It will mark all detected secrets as false positives but only in the context of this specific run - it won’t remember this action in subsequent check runs.
If you encounter any problems you can reach out to us on Slack: #gitguardian-secret-scanning-help
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>
sfc-gh-pjob
approved these changes
Apr 2, 2024
sfc-gh-turbaszek
approved these changes
Apr 2, 2024
sfc-gh-sichen
pushed a commit
that referenced
this pull request
Oct 17, 2024
* Add ignore-anaconda flag * rewrite download_packages to wheels * refactor * parse requirements from wheel files * Read name from wheel metadata * use wheel-convention of package name to lookup metadata * use pip wheel instead of pip download * fix tests * fix user messages * deprecate --pypi-download * fix snowpark flow * add --index-url flag * add --skip-version-check flag * add --allow-shared-libraries flag * refactor package create command * refactor * fix integration tests * fix; fix unit tests * update release notes * fix test_utils.py * fix snowpark build * (probably) fix windows integration tests * remove get_anaconda_from_snowflake * convert --allow-sharted-libraries flag to boolean * add backwards compatibility support * deprecate --pypi-download flag * rename --check-anaconda-for-pypi-depts to --ignore-anaconda * refactor snow snowpark build * remove package argument from download_packages * Update src/snowflake/cli/plugins/snowpark/package_utils.py Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com> * review fixes * replace --package-native-libraries with --allow-shared-libraries * add --skip-version-check and --index-url flags * remove snowpark_package * debug IN PROGRESS * Add tests for snowpark build * turn off anaconda check + add donwload mock to quicken unit tests * remove dedup_and_sort_requirements * get rid of requirements.other.txt * refactor download_packages * get rid of .packages directory * fix tests * update release notes * update release notes * fix tests * review fixes * update release notes * (hopefully) fix integration test for Windows * review fixes * check whether Windows shared libraries detection works * Adjust error messages; use package numpy with --ignore-anaconda flag in tests * Update src/snowflake/cli/plugins/snowpark/commands.py Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com> * review fixes * fix unit tests * style fix --------- Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pre-review checklist
Changes description
Cient-facing changes:
--pypi-downloadflag have no effect and will cause a warning. Create command always check against PyPi.--check-anaconda-for-pypi-deptsis replaced with--ignore-anaconda--package-native-librariesis replaced with boolean flag--allow-shared-libraries--skip-version-checkskips comparing versions of dependencies between requirements and Anaconda.--index-urlflag sets up Base URL of the Python Package Index to use for package lookup.Major code changes:
snow snowpark buildcommand implementation, getting rid ofsnowpark_packagefunction, adjusting flags according to https://docs.google.com/document/d/1yMaD1A5UUQCdeH6p1-IRaYsFTXZFArFAQEkD81Kd57o/edit#heading=h.4fts1bfc5rlopackage_utils.download_packages:List[Requirement]only ->requirements.other.txtis no longer neededsnow snowpark builddedup_and_sort_reqs, as it started conflicting with other formats of requirements (for example github url)