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.

Sign up for GitHub

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

Jump to bottom

Change dirhash package to checksumdir #708

Merged
merged 10 commits into from
Oct 22, 2021
Merged

Change dirhash package to checksumdir #708

merged 10 commits into from
Oct 22, 2021

Conversation

martinlyra
Copy link
Contributor

@martinlyra martinlyra commented Oct 18, 2021 •
edited
Loading

TL;DR

This is a proposed fix to a compatibility issue with Windows as described in this issue: flyteorg/flyte#1561 (with additional details in the Slack thread). In short the issue stems from an package called dirtree which depends on an other package by the same author; scantree. Since the packages are no longer maintained, checksumdir is proposed to replace it.

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

Complete description

The package dirhash which depends on the culprit scantree package is replaced by checksumdir in fast_registration.py

Unfortunately, scantree uses posix directly instead of os which becomes an issue with Windows. After somewhat thorough deliberation in Slack, we reached an impasse that we should replace dirhash with checksumdir and see if it would help resolve the compatibility issue.

In order to improve compatibility on Windows. An explicit encoding="utf-8" is added to an open() in setup.py as well.

Tracking Issue

flyteorg/flyte#1561

Follow-up issue

flyteorg/flyte#1561

@welcome
Copy link

welcome bot commented Oct 18, 2021

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).

@samhita-alla
Copy link
Contributor

@martinlyra, thanks for creating this PR. Can we have dirhash removed and checksumdir added to the requirements.txt, dev-requirements.txt, doc-requirements.txt, setup.py and requirements-spark2.txt files?

cc: @wild-endeavor

@samhita-alla
Copy link
Contributor

Also, sign off your commits by referring to https://github.com/flyteorg/flytekit/pull/708/checks?check_run_id=3929898169.

@martinlyra martinlyra force-pushed the compatibility-test-fix branch from 39ac560 to 58e1d8b Compare October 19, 2021 10:50
@codecov
Copy link

codecov bot commented Oct 19, 2021 •
edited
Loading

Codecov Report

Merging #708 (c63377c) into master (f03ecb6) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #708   +/-   ##
=======================================
  Coverage   85.74%   85.74%           
=======================================
  Files         357      357           
  Lines       29747    29747           
  Branches     2428     2428           
=======================================
  Hits        25507    25507           
  Misses       3601     3601           
  Partials      639      639
Impacted Files Coverage Δ
flytekit/tools/fast_registration.py 41.81% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f03ecb6...c63377c. Read the comment docs.

@martinlyra martinlyra marked this pull request as ready for review October 21, 2021 12:02
samhita-alla
samhita-alla reviewed Oct 21, 2021
View reviewed changes
flytekit/tools/fast_registration.py Outdated
@@ -18,7 +18,7 @@ def compute_digest(source_dir: _os.PathLike) -> str:
:param _os.PathLike source_dir:
:return Text:
"""
return f"fast{_dirhash.dirhash(source_dir, 'md5', match=['*.py'])}"
return f"fast{_dirhash(source_dir, 'md5')}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you simply import checksumdir without an alias and make sure you use chemsumdir.dirhash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved now in c03c6fb

@martinlyra martinlyra force-pushed the compatibility-test-fix branch from ee1ef6d to 90054e7 Compare October 21, 2021 12:53
@martinlyra martinlyra force-pushed the compatibility-test-fix branch from 90054e7 to c03c6fb Compare October 21, 2021 12:58
@martinlyra martinlyra force-pushed the compatibility-test-fix branch from b711676 to d71e7b8 Compare October 21, 2021 14:33
@martinlyra martinlyra force-pushed the compatibility-test-fix branch from 2ed2f25 to c63377c Compare October 22, 2021 14:40
@wild-endeavor wild-endeavor merged commit b48359d into flyteorg:master Oct 22, 2021
@welcome
Copy link

welcome bot commented Oct 22, 2021

Congrats on merging your first pull request! 🎉

reverson pushed a commit to reverson/flytekit that referenced this pull request May 27, 2022
@martinlyra
Change dirhash package to checksumdir (flyteorg#708)
ae26cdd 
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Martin Lyrå <[email protected]>
Signed-off-by: Robert Everson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samhita-alla samhita-alla samhita-alla left review comments

@wild-endeavor wild-endeavor wild-endeavor approved these changes

@eapolinario eapolinario Awaiting requested review from eapolinario eapolinario is a code owner

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinlyra @samhita-alla @wild-endeavor @eapolinario