#1437 Add .gitignore and .dockerignore behavior to packaging

[bimtauer]

TL;DR

Implements flyteorg/flyte#1437 by refactoring the tarfile filter logic to use a group of filter classes. Previous filtering logic was preserved in StandardIgnore.

Decided to not go with a separate .flyteignore. Covering .gitignore and .dockerignore if present should work well with a users intentions and avoids having to manage yet another file. If its in .gitignore, you probably dont want it in source control (from where you probably build at least your production images) and if its in .dockerignore you probably dont want it in the task-running container anyways. So following these two should be good :)
And well if you like it messy, then as before, the in-built filtering saves you from packaging cache and pyc stuff.

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 GitIgnore checks for the presence of git cli and then uses that to check whether a path is ignored. Given the complexity of gitignores with the possibility of nesting and various wildcards, this was the safest way to reliable reproduce actual gitignore behavior.
The DockerIgnore uses the PatternMatcher from docker-py to match file paths against patterns read from a .dockerignore if present. Docker only takes into accout .dockerignore files at the root of the build context, not in subdirectories, so we only check for that. The behavior should thus also be identical to what docker build would include.

The archiving code was refactored into create_archive for less repetition and separate unit testing.
The previous filter function and test was removed since the new feature covers that functionality.

Tracking Issue

flyteorg/flyte#1437

Follow-up issue

NA

[wild-endeavor]

Thank you for the PR! I will review this weekend.

[wild-endeavor]

Are all the subprocess calls okay? Some of these repos might get pretty large - subprocess calling git on every single file, is that UX okay?

[kumare3]

Are all the subprocess calls okay? Some of these repos might get pretty large - subprocess calling git on every single file, is that UX okay?

@bimtauer I like this code. But @wild-endeavor is right, I see you have implemented pattern matcher, why not use the same for git?

[kumare3]

Are all the subprocess calls okay? Some of these repos might get pretty large - subprocess calling git on every single file, is that UX okay?

@bimtauer I like this code. But @wild-endeavor is right, I see you have implemented pattern matcher, why not use the same for git?

Also the patterns are listed here. Only weird one is !

[kumare3]

not your code, but i think compute_digest is wrong!. It is computing on the entire source and not the filtered objects only

[bimtauer]

Are all the subprocess calls okay? Some of these repos might get pretty large - subprocess calling git on every single file, is that UX okay?

@bimtauer I like this code. But @wild-endeavor is right, I see you have implemented pattern matcher, why not use the same for git?

Cannot use that for git as gitignore pattern matching is slightly different from dockerignore (some examples here) and also git allows for multiple nested gitignores.
But I followed @wild-endeavor's advice and switched from many subprocess calls to a single one with git ls-files -io. That accumulates all ignored files once. Only small issue: this does not list entirely ignored directories, only files within them. As a result those directories turn up in the tarball but empty. Should be an okay compromise?

I'll look at the digest asap, good catch!

[codecov]

Codecov Report

Merging #899 (5aa1a15) into master (3cfbb71) will increase coverage by 0.14%.
The diff coverage is 95.43%.

@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
+ Coverage   86.43%   86.57%   +0.14%     
==========================================
  Files         243      245       +2     
  Lines       23243    23483     +240     
  Branches     2618     2645      +27     
==========================================
+ Hits        20089    20330     +241     
+ Misses       2713     2699      -14     
- Partials      441      454      +13     

Impacted Files	Coverage Δ
flytekit/tools/repo.py	46.03% <0.00%> (+1.25%)	⬆️
flytekit/clis/sdk_in_container/serialize.py	56.52% <33.33%> (+1.85%)	⬆️
flytekit/tools/ignore.py	90.69% <90.69%> (ø)
flytekit/tools/fast_registration.py	84.90% <92.68%> (+41.26%)	⬆️
...ests/flytekit/unit/tools/test_fast_registration.py	100.00% <100.00%> (ø)
tests/flytekit/unit/tools/test_ignore.py	100.00% <100.00%> (ø)
tests/flytekit/unit/extras/tasks/test_shell.py	82.17% <0.00%> (-17.83%)	⬇️
flytekit/extras/persistence/http.py	81.57% <0.00%> (-2.64%)	⬇️
flytekit/extras/tasks/shell.py	84.37% <0.00%> (-1.57%)	⬇️

---

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 3cfbb71...5aa1a15. Read the comment docs.

[bimtauer]

Developed update to include only not ignored files in checksum. But unfortunately the exclude_files of checksumdironly checks for file basenames, not for relative paths 🤦🏼 I'm hesitant to strip relative path from the paths to check, since foo/bar.pymight be ignored but bar.py might not be. I'm tempted to write my own quick checksum func and eliminate this dependency. Wdyt?

Also having a look at the failing windows tests. I dont have access to a windows machine 🤢 but I'll try my best 🐧

[bimtauer]

@wild-endeavor @kumare3 Bigger refactoring:

• eliminated subprocesses (only one left) but keep ignoring empty dirs 💪🏼
• performance improvement through dict key lookup in gitignore - its still marginally slower than old filter but overall usually faster because fewer files to package
• Integrated with latest master changes
• Began fixing requirements (pinning websocket-client) but grpcio io yank is still making waves through dep tree. Cannot lock everything, currently builds are failing.

PS might need some help on windows debugging.

[kumare3]

this is great

[kumare3]

@bimtauer some tests are failing and merge conflicts. But this otherwise looks awesome 🙏🏽

[bimtauer]

@kumare3 I need help debugging the windows tests. I don't have access to a windows dev environment and cannot figure out why the tests fail there. Otherwise this should be ready.

[eapolinario]

@bimtauer , #967 was supposed to be a stacked PR on top of this PR, but I botched it. In any case, feel free to cherry-pick the specific commits that fix the unit failures on windows. I also took the opportunity to resolve the conflict and merge master.

[eapolinario]

This change ended up being merged in #967.

@bimtauer , thanks for your contribution!

[bimtauer]

@eapolinario Thank you for pushing it over the finish line!