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

Added symlink dereferencing in fast packaging and tests #1151

Merged
merged 3 commits into from
Sep 9, 2022

Conversation

vchowdhary
Copy link
Contributor

TL;DR

Adds a switch to enable dereferencing symlinks during fast packaging in pyflyte serialize fast workflows.
cc @wild-endeavor

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added --- added documentation for click switch
  • Any pending items have an associated Issue

Complete description

Added a click switch to pyflyte serialize fast workflows that enables/disables dereferencing any symlinks encountered during packaging. This only applies to fast_package.

Tracking Issue

NA

No issue associated with this -- this is part of a request from Stripe. Our use case for this is as follows:
Our workflows often import code from utils files that are in sibling directories -- this makes the current pyflyte package not quite work for us. Our workaround is to use our bazel build target, which generates a runfiles directory that contains all the source code and dependencies, and then serialize from that folder. Some symlinks are generated during the build, so we needed to add dereference=True when opening the tar file so that the actual target contents are loaded and downloaded during execution.

This change should not effect any of the current DataPersistence plugins that are used since they are all remote and would require dereferencing symlinks anyways if they wanted to use the contents.

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Sep 8, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Signed-off-by: Vanshika Chowdhary <[email protected]>
@vchowdhary vchowdhary force-pushed the vchowdhary/deref-symlinks branch from 07bfd1e to 7227af8 Compare September 8, 2022 17:34
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #1151 (9c133d8) into master (61bd02a) will increase coverage by 0.07%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #1151      +/-   ##
==========================================
+ Coverage   68.43%   68.51%   +0.07%     
==========================================
  Files         288      288              
  Lines       26032    26085      +53     
  Branches     2915     2918       +3     
==========================================
+ Hits        17816    17871      +55     
+ Misses       7736     7735       -1     
+ Partials      480      479       -1     
Impacted Files Coverage Δ
flytekit/clis/sdk_in_container/serialize.py 57.14% <66.66%> (+0.62%) ⬆️
flytekit/clis/sdk_in_container/package.py 96.29% <100.00%> (+0.14%) ⬆️
flytekit/clis/sdk_in_container/register.py 80.76% <100.00%> (+0.37%) ⬆️
flytekit/tools/fast_registration.py 89.06% <100.00%> (ø)
flytekit/tools/repo.py 73.68% <100.00%> (ø)
...ests/flytekit/unit/tools/test_fast_registration.py 100.00% <100.00%> (ø)
tests/flytekit/unit/core/test_type_engine.py 98.39% <0.00%> (+<0.01%) ⬆️
flytekit/core/type_engine.py 59.34% <0.00%> (+0.09%) ⬆️
tests/flytekit/unit/cli/pyflyte/test_run.py 99.20% <0.00%> (+0.32%) ⬆️
flytekit/clis/sdk_in_container/run.py 84.19% <0.00%> (+0.59%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

This change looks good. Can you also make sure that new flag is added to the other places where fast_package is used?

Signed-off-by: Vanshika Chowdhary <[email protected]>
@vchowdhary vchowdhary force-pushed the vchowdhary/deref-symlinks branch from 43ac059 to 957d18a Compare September 8, 2022 23:26
@vchowdhary
Copy link
Contributor Author

This change looks good. Can you also make sure that new flag is added to the other places where fast_package is used?

Thanks for catching that! There was one place that I missed.

@eapolinario
Copy link
Collaborator

eapolinario commented Sep 9, 2022

What about?

archive_fname = fast_registration.fast_package(source, output_tmpdir)

Signed-off-by: Vanshika Chowdhary <[email protected]>
@vchowdhary
Copy link
Contributor Author

Ah my bad, missed that.

@eapolinario eapolinario merged commit 1c75bd1 into flyteorg:master Sep 9, 2022
@welcome
Copy link

welcome bot commented Sep 9, 2022

Congrats on merging your first pull request! 🎉

@vchowdhary
Copy link
Contributor Author

@eapolinario
Any idea on when this would be released? We see that there are monthly releases usually and one hasn't happened yet for september.

eapolinario pushed a commit that referenced this pull request Sep 15, 2022
* Added symlink dereferencing and tests

Signed-off-by: Vanshika Chowdhary <[email protected]>

* Added flag to register as well

Signed-off-by: Vanshika Chowdhary <[email protected]>

* More flag propagation

Signed-off-by: Vanshika Chowdhary <[email protected]>

Signed-off-by: Vanshika Chowdhary <[email protected]>
Co-authored-by: Vanshika Chowdhary <[email protected]>
eapolinario added a commit that referenced this pull request Sep 16, 2022
* Add deck to papermill plugin task (#1111)

Signed-off-by: Calvin Leather <[email protected]>

* Run compilation even in local execution for dynamic tasks to early detect errors (#1121)

Signed-off-by: Yee Hing Tong <[email protected]>

* Set to pyflyte run blob object remote when dealing with remote files (#1128)

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>

* Override voidPromise resource (#1127)

* override void promise resource

Signed-off-by: Kevin Su <[email protected]>

* override void promise resource

Signed-off-by: Kevin Su <[email protected]>

* Fix how ShellTask retrieves the Pod class name (#1132)

* Fix how ShellTask retrieves the Pod class name

Signed-off-by: Matheus Moreno <[email protected]>

* Set Pod class name as a constant

Signed-off-by: Matheus Moreno <[email protected]>

* Revert last commit

Signed-off-by: Matheus Moreno <[email protected]>

* Execute automatic linting

Signed-off-by: Matheus Moreno <[email protected]>

Signed-off-by: Matheus Moreno <[email protected]>

* Add restriction for pandas to be >=1.2 for fsspec plugin (#1136)

Signed-off-by: Yee Hing Tong <[email protected]>

* Use joblib hashing to generate cache key to ensure repeatability (#1126)

* cherry pick 97b454b

Signed-off-by: Yee Hing Tong <[email protected]>

* requirements

Signed-off-by: Yee Hing Tong <[email protected]>

* Fix usage of save in ProtoJoblibHasher

Signed-off-by: Eduardo Apolinario <[email protected]>

* Regenerate requirements using python 3.7

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add test_stable_cache_key

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Allow None protocol to mean all data persistence supported storage options in Structured Dataset (#1134)

Signed-off-by: Yee Hing Tong <[email protected]>

* handle ImportError and OSError in extras.pytorch (#1141)

* handle ImportError and OSError in extras.pytorch

Signed-off-by: Niels Bantilan <[email protected]>

* isolate exception to torch import

Signed-off-by: Niels Bantilan <[email protected]>

Signed-off-by: Niels Bantilan <[email protected]>

* Register dataframe renderers in structured dataset (#1140)

* Register dataframe renderers in structured dataset

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix test

Signed-off-by: Kevin Su <[email protected]>

* more tests

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* pyflyte run imperative workflows (#1131)

Signed-off-by: Kevin Su <[email protected]>

* Using sidecar handler to run Papermill task (#1143)

* remove nb prefix

Signed-off-by: Kevin Su <[email protected]>

* add tests

Signed-off-by: Kevin Su <[email protected]>

* Update requirements.in

Signed-off-by: Kevin Su <[email protected]>

* remove _

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Properly raise error in NumpyArrayTransformer (#1146)

Signed-off-by: Rahul Mehta <[email protected]>

Signed-off-by: Rahul Mehta <[email protected]>

* Add assert_type in dataclass transformer (#1149)

* Add assert_type in dataclassTransformer

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* more tests

Signed-off-by: Kevin Su <[email protected]>

* fix lint

Signed-off-by: Kevin Su <[email protected]>

* Add one more test

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Pickle in Union Type (#1147)

* Pickel in Union type

Signed-off-by: Kevin Su <[email protected]>

* Pickel in Union type

Signed-off-by: Kevin Su <[email protected]>

* wip

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* update tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* fix tests

Signed-off-by: Kevin Su <[email protected]>

* Address comment

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Bump max docker version to 7.0.0 (#1138)

Signed-off-by: Rahul Mehta <[email protected]>

Signed-off-by: Rahul Mehta <[email protected]>

* Set flytekit<2.0 in plugins (#1152)

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Add literal type to union literal (#1144)

* Add literal type to union literal

Signed-off-by: Kevin Su <[email protected]>

* fix test

Signed-off-by: Kevin Su <[email protected]>

* Add tests

Signed-off-by: Kevin Su <[email protected]>

* more tests

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Fix the type of optional[int] in nested dataclass (#1148)

* Fix the type of optional[int] in nested dataclass

Signed-off-by: Kevin Su <[email protected]>

* update tests

Signed-off-by: Kevin Su <[email protected]>

* update comments

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* Added symlink dereferencing in fast packaging and tests (#1151)

* Added symlink dereferencing and tests

Signed-off-by: Vanshika Chowdhary <[email protected]>

* Added flag to register as well

Signed-off-by: Vanshika Chowdhary <[email protected]>

* More flag propagation

Signed-off-by: Vanshika Chowdhary <[email protected]>

Signed-off-by: Vanshika Chowdhary <[email protected]>
Co-authored-by: Vanshika Chowdhary <[email protected]>

* Strip newline from client secret (#1163)

* Strip newline from client secret

* Add logging and rework the secret file comparison to work on windows

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Fix the type of optional[int] in dataclass (#1135)

Signed-off-by: Kevin Su <[email protected]>

* fix: plugins/flytekit-papermill/dev-requirements.txt to reduce vulnerabilities (#1154)

The following vulnerabilities are fixed by pinning transitive dependencies:
- https://snyk.io/vuln/SNYK-PYTHON-OAUTHLIB-3021142
- https://snyk.io/vuln/SNYK-PYTHON-PYSPARK-3021131

Signed-off-by: Eduardo Apolinario <[email protected]>

* Using sidecar handler to run Papermill task (#1143)

* remove nb prefix

Signed-off-by: Kevin Su <[email protected]>

* add tests

Signed-off-by: Kevin Su <[email protected]>

* Update requirements.in

Signed-off-by: Kevin Su <[email protected]>

* remove _

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

* fix: plugins/flytekit-papermill/dev-requirements.txt to reduce vulnerabilities (#1145)

The following vulnerabilities are fixed by pinning transitive dependencies:
- https://snyk.io/vuln/SNYK-PYTHON-COOKIECUTTER-2414281

* Bump pyspark from 3.2.1 to 3.2.2 in /plugins/flytekit-papermill (#1130)

Bumps [pyspark](https://github.com/apache/spark) from 3.2.1 to 3.2.2.
- [Release notes](https://github.com/apache/spark/releases)
- [Commits](apache/spark@v3.2.1...v3.2.2)

---
updated-dependencies:
- dependency-name: pyspark
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: plugins/flytekit-papermill/dev-requirements.txt to reduce vulnerabilities (#1154)

The following vulnerabilities are fixed by pinning transitive dependencies:
- https://snyk.io/vuln/SNYK-PYTHON-OAUTHLIB-3021142
- https://snyk.io/vuln/SNYK-PYTHON-PYSPARK-3021131

Signed-off-by: Calvin Leather <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Matheus Moreno <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Rahul Mehta <[email protected]>
Signed-off-by: Vanshika Chowdhary <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Calvin Leather <[email protected]>
Co-authored-by: Yee Hing Tong <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: Matheus Moreno <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Niels Bantilan <[email protected]>
Co-authored-by: Rahul Mehta <[email protected]>
Co-authored-by: Vanshika Chowdhary <[email protected]>
Co-authored-by: Vanshika Chowdhary <[email protected]>
Co-authored-by: Snyk bot <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants