Fix remote file system (get/put) #1955

pingsutw · 2023-11-13T23:08:17Z

TL;DR

Failed to serialize the flytefile due to below error:

[3/3] currentAttempt done. Last Error: SYSTEM::Traceback (most recent call last):

      File "/usr/local/lib/python3.10/site-packages/flytekit/exceptions/scopes.py", line 165, in system_entry_point
        return wrapped(*args, **kwargs)
      File "/usr/local/lib/python3.10/site-packages/flytekit/core/base_task.py", line 662, in dispatch_execute
        literals_map, native_outputs_as_map = self._output_to_literal_map(native_outputs, exec_ctx)
      File "/usr/local/lib/python3.10/site-packages/flytekit/core/base_task.py", line 561, in _output_to_literal_map
        raise TypeError(msg)

Message:

    Failed to convert outputs of task 'test.t1' at position 0:
  'list' object has no attribute 'startswith'

SYSTEM ERROR! Contact platform administrators.

That's because asyncFileSystem (s3fs) return [None] when calling file_system.put here

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

from flytekit import task, ImageSpec, workflow
from flytekit.types.file import FlyteFile

new_flytekit = "git+https://github.com/flyteorg/flytekit.git@c49ceb779cb4617053004c94d4bd71b0622c63b2"
image = ImageSpec(apt_packages=["git"], packages=[new_flytekit], registry="pingsutw")


@task(container_image=image)
def t1() -> FlyteFile:
    tmp_file = "/tmp/abc"
    with open(tmp_file, "w") as f:
        f.write("wow")
    return tmp_file


@workflow()
def wf():
    t1()


if __name__ == '__main__':
    wf()

Tracking Issue

NA

Follow-up issue

NA

Signed-off-by: Kevin Su <[email protected]>

codecov · 2023-11-13T23:43:56Z

Codecov Report

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

Comparison is base (38c7687) 62.70% compared to head (cd2cbc7) 62.58%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
flytekit/core/data_persistence.py	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1955      +/-   ##
==========================================
- Coverage   62.70%   62.58%   -0.13%     
==========================================
  Files         313      310       -3     
  Lines       23181    23108      -73     
  Branches     3511     3513       +2     
==========================================
- Hits        14536    14462      -74     
- Misses       8223     8224       +1     
  Partials      422      422

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

pingsutw · 2023-11-14T00:10:09Z

cc @kumare3 @wild-endeavor @eapolinario

wild-endeavor

is there a unit test we can add?

wild-endeavor · 2023-11-14T05:58:42Z

flytekit/core/data_persistence.py

@@ -248,7 +248,10 @@ def get(self, from_path: str, to_path: str, recursive: bool = False, **kwargs):
                    self.strip_file_header(from_path), self.strip_file_header(to_path), dirs_exist_ok=True
                )
            print(f"Getting {from_path} to {to_path}")
-            return file_system.get(from_path, to_path, recursive=recursive, **kwargs)
+            dst = file_system.get(from_path, to_path, recursive=recursive, **kwargs)


what does dst stand for?

maybe add a test like

flytekit/tests/flytekit/unit/core/test_type_engine.py

Line 203 in 38c7687

class JsonTypeTransformer(TypeTransformer[T]):

, just making a dummy transformer, if that's easy to do. if not, just add the below as a comment in both places.

dst is short for destination, right?

yes, since fsspec uses dst as well

wild-endeavor · 2023-11-14T06:06:50Z

@eapolinario basically with the pr that went in we kinda added a new API to the api without being able to make it explicit.

Basically, when an fsspec filesystem now handles get/put, it has the option of returning a value, and that value is now also used by flytekit as the uri in most places. This was useful because the flyte fs is not a real fs, so users don't actually pick the to_path that they want to write to. instead where it was actually written is returned by the filesystem.

In this case however, the s3 filesystem was returning [None], so the replacement mechanism was failing. (the replacement mechanism is there because in oss flyte at least, even if the user opts in to write to the flyte remote fs, you don't want to store flyte:// as the uri. you still want to store the underlying s3/gcs path).

eapolinario · 2023-11-14T06:56:11Z

flytekit/core/data_persistence.py

@@ -248,7 +248,10 @@ def get(self, from_path: str, to_path: str, recursive: bool = False, **kwargs):
                    self.strip_file_header(from_path), self.strip_file_header(to_path), dirs_exist_ok=True
                )
            print(f"Getting {from_path} to {to_path}")
-            return file_system.get(from_path, to_path, recursive=recursive, **kwargs)
+            dst = file_system.get(from_path, to_path, recursive=recursive, **kwargs)


dst is short for destination, right?

eapolinario · 2023-11-14T07:24:31Z

flytekit/core/data_persistence.py

@@ -248,7 +248,10 @@ def get(self, from_path: str, to_path: str, recursive: bool = False, **kwargs):
                    self.strip_file_header(from_path), self.strip_file_header(to_path), dirs_exist_ok=True
                )
            print(f"Getting {from_path} to {to_path}")
-            return file_system.get(from_path, to_path, recursive=recursive, **kwargs)
+            dst = file_system.get(from_path, to_path, recursive=recursive, **kwargs)
+            if isinstance(dst, (str, pathlib.Path)):


IIUC, [None] is a guard value returned by fsspec, right? If that's the case, why don't we handle those explicitly?

oh really? didn't see that. link me?

and yes we should, but the action should still be to return the to_path.

I don't think it's part of the spec and I fear this is specific to s3fs. Doing what we're doing in the PR is probably safer (as it might apply to other fsspec-compliant implementations).

AsyncFileSystem.put or AsyncFileSystem.get always return a list.

https://github.com/fsspec/filesystem_spec/blob/294248578994de67f8da00692de6872f1352ce33/fsspec/asyn.py#L574

https://github.com/fsspec/filesystem_spec/blob/294248578994de67f8da00692de6872f1352ce33/fsspec/asyn.py#L242-L256

Signed-off-by: Kevin Su <[email protected]>

* test Signed-off-by: Kevin Su <[email protected]> * test Signed-off-by: Kevin Su <[email protected]> * test Signed-off-by: Kevin Su <[email protected]> * lint Signed-off-by: Kevin Su <[email protected]> * update-get Signed-off-by: Kevin Su <[email protected]> * add unit test Signed-off-by: Kevin Su <[email protected]> --------- Signed-off-by: Kevin Su <[email protected]>

* test Signed-off-by: Kevin Su <[email protected]> * test Signed-off-by: Kevin Su <[email protected]> * test Signed-off-by: Kevin Su <[email protected]> * lint Signed-off-by: Kevin Su <[email protected]> * update-get Signed-off-by: Kevin Su <[email protected]> * add unit test Signed-off-by: Kevin Su <[email protected]> --------- Signed-off-by: Kevin Su <[email protected]> Signed-off-by: Rafael Raposo <[email protected]>

pingsutw added 5 commits November 13, 2023 14:02

test

7b19428

Signed-off-by: Kevin Su <[email protected]>

test

e630b71

Signed-off-by: Kevin Su <[email protected]>

test

c49ceb7

Signed-off-by: Kevin Su <[email protected]>

lint

bad6378

Signed-off-by: Kevin Su <[email protected]>

update-get

22083b0

Signed-off-by: Kevin Su <[email protected]>

pingsutw requested review from wild-endeavor, kumare3, eapolinario and cosmicBboy as code owners November 13, 2023 23:08

wild-endeavor previously approved these changes Nov 14, 2023

View reviewed changes

eapolinario reviewed Nov 14, 2023

View reviewed changes

add unit test

cd2cbc7

Signed-off-by: Kevin Su <[email protected]>

pingsutw dismissed wild-endeavor’s stale review via cd2cbc7 November 14, 2023 19:41

eapolinario approved these changes Nov 14, 2023

View reviewed changes

eapolinario merged commit 28adeee into master Nov 14, 2023
70 of 72 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix remote file system (get/put) #1955

Fix remote file system (get/put) #1955

pingsutw commented Nov 13, 2023

codecov bot commented Nov 13, 2023 •

edited

Loading

pingsutw commented Nov 14, 2023

wild-endeavor left a comment

wild-endeavor Nov 14, 2023

wild-endeavor Nov 14, 2023

eapolinario Nov 14, 2023

pingsutw Nov 14, 2023

wild-endeavor commented Nov 14, 2023

eapolinario Nov 14, 2023

eapolinario Nov 14, 2023

wild-endeavor Nov 14, 2023

wild-endeavor Nov 14, 2023

eapolinario Nov 14, 2023

pingsutw Nov 14, 2023

Fix remote file system (get/put) #1955

Fix remote file system (get/put) #1955

Conversation

pingsutw commented Nov 13, 2023

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

pingsutw commented Nov 14, 2023

wild-endeavor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wild-endeavor commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2023 •

edited

Loading