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

Use shutil instead of setuptools/distutils to copy dirs #1349

Merged
merged 10 commits into from
Nov 28, 2022

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Nov 22, 2022

TL;DR

The bump up recently to setuptools version from 65.5.1 to 65.6.0 caused one of our click unit tests to fail - some interaction between the directory copy utility provided by setuptools.distutils and the python logging library.

Getting around this by using shutil instead.

Also realized that

  • FlyteSchema transformers doesn't currently handle well you
    @task
    def t3(df: FlyteSchema) -> FlyteSchema:
        return df
    Because it's never downloaded, there's nothing to upload back. Added a special case to prevent this.
  • Nested dataclasses will get transformed multiple times (at least the same FlyteSchema instance will) by the type engine. Something to fix for the future.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

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

Tracking Issue

Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor changed the title use shutil instead of setuptools to copy dirs use shutil instead of setuptools/distutils to copy dirs Nov 22, 2022
@wild-endeavor wild-endeavor changed the title use shutil instead of setuptools/distutils to copy dirs Use shutil instead of setuptools/distutils to copy dirs Nov 22, 2022
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #1349 (ce4f474) into master (7cf5b68) will decrease coverage by 1.40%.
The diff coverage is 67.85%.

@@            Coverage Diff             @@
##           master    #1349      +/-   ##
==========================================
- Coverage   70.41%   69.01%   -1.41%     
==========================================
  Files         291      291              
  Lines       25594    26679    +1085     
  Branches     2129     2513     +384     
==========================================
+ Hits        18023    18413     +390     
- Misses       7083     7772     +689     
- Partials      488      494       +6     
Impacted Files Coverage Δ
flytekit/core/data_persistence.py 35.52% <57.89%> (+1.40%) ⬆️
flytekit/types/schema/types.py 38.30% <75.00%> (-1.52%) ⬇️
tests/flytekit/unit/cli/pyflyte/test_run.py 99.20% <100.00%> (+0.07%) ⬆️
tests/flytekit/unit/core/tracker/d.py 40.00% <0.00%> (-10.00%) ⬇️
flytekit/models/execution.py 42.20% <0.00%> (-9.43%) ⬇️
flytekit/models/core/workflow.py 42.63% <0.00%> (-9.26%) ⬇️
flytekit/models/core/identifier.py 38.70% <0.00%> (-8.66%) ⬇️
flytekit/models/dynamic_job.py 37.50% <0.00%> (-8.66%) ⬇️
flytekit/models/project.py 34.28% <0.00%> (-8.58%) ⬇️
flytekit/models/literals.py 40.28% <0.00%> (-8.32%) ⬇️
... and 155 more

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

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
eapolinario
eapolinario previously approved these changes Nov 25, 2022
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor merged commit a47e383 into master Nov 28, 2022
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