Fix file not found error by austin362667 · Pull Request #2079 · flyteorg/flytekit

austin362667 · 2024-01-02T08:01:30Z

Tracking issue

Why are the changes needed?

Flytekit should raise a file not found error instead of Access denied

What changes were proposed in this pull request?

Raise an exception if the path doesn't exist on the filesystem when dst = file_system.get(from_path, to_path, recursive=recursive, **kwargs) catches OSError.

How was this patch tested?

Setup process

As per mentioned by flyteorg/flyte#4470

pyflyte run --remote workflow.py wf
remove script_mode.tar.gz from s3 bucket (e.g., via minio)
pyflyte run --remote workflow.py wf # run the workflow again

Screenshots

Check all the applicable boxes

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

Related PRs

Docs link

welcome · 2024-01-02T08:01:33Z

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

pingsutw · 2024-01-02T19:11:04Z

        except OSError as oe:
            logger.debug(f"Error in getting {from_path} to {to_path} rec {recursive} {oe}")
+            if not file_system.exists(from_path):
+                raise Exception(f"File not found in {from_path}")


nit:

Suggested change

raise Exception(f"File not found in {from_path}")

raise FlyteValueException(f"File not found in {from_path}")

pingsutw · 2024-01-02T19:11:18Z

        v = os.environ.get(env_var)
        if v is not None:
-            return v
+            return v.strip()


why removes strip()?

I think it's because of this PR.
https://github.com/flyteorg/flytekit/pull/2077/files

dropped 😅

codecov · 2024-01-02T19:18:21Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e107994) 85.71% compared to head (e05a9e8) 85.75%.
Report is 5 commits behind head on master.

Files	Patch %	Lines
flytekit/core/data_persistence.py	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2079      +/-   ##
==========================================
+ Coverage   85.71%   85.75%   +0.03%     
==========================================
  Files         313      313              
  Lines       23396    23428      +32     
  Branches     3501     3511      +10     
==========================================
+ Hits        20055    20090      +35     
+ Misses       2733     2731       -2     
+ Partials      608      607       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Future-Outlier

LGTM

Signed-off-by: Austin Liu <austin362667@gmail.com> reorder import Signed-off-by: Austin Liu <austin362667@gmail.com> fix received_value Signed-off-by: Austin Liu <austin362667@gmail.com>

austin362667 · 2024-01-03T11:03:31Z

pingsutw · 2024-01-03T11:24:36Z

            return to_path
        except OSError as oe:
            logger.debug(f"Error in getting {from_path} to {to_path} rec {recursive} {oe}")
+            if not file_system.exists(from_path):


is it possible to raise an error without an additional network call? e.g. are we able to check the error code (404) from oe?

if not, that's fine

Neat, but I've tried oe.errno == FileNotFoundError.errno: and it failed to catch the 404 error.

I think it's ignorable wrt the performance issue, since it's already trapped in the exception handling process.

Future-Outlier

LGTM, wait for your error code reply!

welcome · 2024-01-08T20:21:34Z

Congrats on merging your first pull request! 🎉

austin362667 requested review from cosmicBboy, eapolinario, kumare3, pingsutw and wild-endeavor as code owners January 2, 2024 08:01

austin362667 force-pushed the bug/fs branch from 1787390 to 05a6dbd Compare January 2, 2024 15:20

pingsutw previously approved these changes Jan 2, 2024

View reviewed changes

austin362667 dismissed pingsutw’s stale review via 0711848 January 3, 2024 05:43

austin362667 force-pushed the bug/fs branch 2 times, most recently from 0711848 to ae24e12 Compare January 3, 2024 05:48

Future-Outlier previously approved these changes Jan 3, 2024

View reviewed changes

add fs path exists checking (fix flyteorg/flyte#4470)

e05a9e8

Signed-off-by: Austin Liu <austin362667@gmail.com> reorder import Signed-off-by: Austin Liu <austin362667@gmail.com> fix received_value Signed-off-by: Austin Liu <austin362667@gmail.com>

austin362667 dismissed Future-Outlier’s stale review via e05a9e8 January 3, 2024 06:06

austin362667 force-pushed the bug/fs branch from ae24e12 to e05a9e8 Compare January 3, 2024 06:06

austin362667 marked this pull request as draft January 3, 2024 06:19

austin362667 marked this pull request as ready for review January 3, 2024 11:05

austin362667 requested review from Future-Outlier and pingsutw January 3, 2024 11:05

pingsutw previously approved these changes Jan 3, 2024

View reviewed changes

Future-Outlier previously approved these changes Jan 3, 2024

View reviewed changes

austin362667 marked this pull request as draft January 3, 2024 13:15

austin362667 dismissed stale reviews from Future-Outlier and pingsutw via 67f80b6 January 3, 2024 13:27

austin362667 force-pushed the bug/fs branch from 67f80b6 to e05a9e8 Compare January 3, 2024 14:27

austin362667 marked this pull request as ready for review January 3, 2024 14:31

pingsutw approved these changes Jan 8, 2024

View reviewed changes

pingsutw merged commit f22cec2 into flyteorg:master Jan 8, 2024

	raise Exception(f"File not found in {from_path}")
	raise FlyteValueException(f"File not found in {from_path}")

Uh oh!

Conversation

austin362667 commented Jan 2, 2024

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Uh oh!

welcome Bot commented Jan 2, 2024

Uh oh!

pingsutw Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

pingsutw Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

Future-Outlier Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

austin362667 Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Future-Outlier left a comment

Choose a reason for hiding this comment

Uh oh!

austin362667 commented Jan 3, 2024

Uh oh!

pingsutw Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

pingsutw Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

austin362667 Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Future-Outlier left a comment

Choose a reason for hiding this comment

Uh oh!

welcome Bot commented Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jan 2, 2024 •

edited

Loading

austin362667 Jan 3, 2024 •

edited

Loading