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

Add functionality for .flyteignore file #2479

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

dansola
Copy link
Contributor

@dansola dansola commented Jun 13, 2024

Tracking issue

Linear Ticket: https://linear.app/unionai/issue/COR-1133/add-flyteignore-functionality-to-pyflyte-register

Why are the changes needed?

Sometimes users have files they want to ignore in the registration process that do not suit a dockerignore or gitignore.

What changes were proposed in this pull request?

Add a FlyteIgnore class that looks for a .flyteignore class in the root directory and ignores any files in it when computing a hash.

How was this patch tested?

  • Unit tests.
  • Local scenario with symlink:
.
├── lp.py
├── original_link.txt -> /Users/danielsola/repos/wine-classification/original.txt
└── wf.py

lp.py is

from flytekit import LaunchPlan
from wf import hello_world_wf

LaunchPlan.get_or_create(
    hello_world_wf,
    name="hello_lp",
)

and wf.py is

from flytekit import task, workflow

@task
def say_hello() -> str:
    return "Hello, World!"

@workflow
def hello_world_wf() -> str:
    res = say_hello()
    return res

Then I create a random file like this:
echo "This is the original file." > ~/path/to/repo/original.txt
and the symlink in the directory I showed you like this:
ln -s ~/repos/wine-classification/original.txt ~/path/to/repo/register_example/original_link.txt
Then I move the original file like this:
mv ~/path/to/repo/original.txt ~/path/to/repo/original-moved.txt
and then run
pyflyte register lp.py

With this I get an error
Failed with Unknown Exception <class 'FileNotFoundError'> Reason: [Errno 2] No such file or directory: '/path/to/repo/register_example/original_link.txt'

Then I add *.txt to a .flyteignore and I no longer get a failure.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Original PR for gitignore and dockerignore: #899

Docs link

squiishyy
squiishyy previously approved these changes Jun 14, 2024
flyteignore = os.path.join(self.root, ".flyteignore")
if os.path.isfile(flyteignore):
with open(flyteignore, "r") as f:
patterns = [l.strip() for l in f.readlines() if l.strip() and not l.startswith("#")]
Copy link
Contributor

Choose a reason for hiding this comment

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

we may end up removing the dependency in the future, but while we have it, why not just use the PatternMatcher object that the dockerignore object uses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha just pushed that! since my test was actually failing

@wild-endeavor
Copy link
Contributor

This looks good to merge, and this is not the first request we've heard about having a separate flyteignore file, but a couple things

  • this seems like the wrong solution to the particular issue mentioned above. The real bug is in the mechanism in flytekit that was looking for a symlinked file that no longer exists. That should be fixed in a separate PR (@dansola we can make another GH issue for that later).
  • This is pretty naive ignoring and it may be worth it to mention that somewhere in a comment - proper git ignores support nested gitignore files, reverse ignoring, etc.

@dansola dansola merged commit 4b2c06b into master Jun 14, 2024
45 of 46 checks passed
Copy link

welcome bot commented Jun 14, 2024

Congrats on merging your first pull request! 🎉

bgedik pushed a commit to bgedik/flytekit that referenced this pull request Jul 3, 2024
* Add functionality for .flyteignore file

Signed-off-by: Daniel Sola <[email protected]>

* copy docker ignore for flyteignore

---------

Signed-off-by: Daniel Sola <[email protected]>
Signed-off-by: bugra.gedik <[email protected]>
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
* Add functionality for .flyteignore file

Signed-off-by: Daniel Sola <[email protected]>

* copy docker ignore for flyteignore

---------

Signed-off-by: Daniel Sola <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
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.

3 participants