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

[BUG] Flytekit fails with long presigned URLs #4701

Open
2 tasks done
cgrass opened this issue Jan 9, 2024 · 3 comments
Open
2 tasks done

[BUG] Flytekit fails with long presigned URLs #4701

cgrass opened this issue Jan 9, 2024 · 3 comments
Assignees
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working

Comments

@cgrass
Copy link
Contributor

cgrass commented Jan 9, 2024

Describe the bug

Long URLs (beyond filesystem limits) break fsspec get operations. I am discussing options with the fsspec community, but the current workaround would require a fix to flytekit and is not ideal. The bug only pops when all of the below are true
a) remoteData is configured to set signedUrls:true
b) the presigned URL is larger than n (file system limit) characters (common for S3)
c) the payload is larger than maxSizeInBytes
d) FlyteRemote is used to interact with the execution - I am not 100% confident in this, but it's the only use case I could find

This is related to another bug I filed and could potentially be fixed with the same solution.

Expected behavior

remote.fetch_execution() succeeds

Additional context to reproduce

@wild-endeavor was able to reproduce using a local setup and the following test:

def test_file_handling_http():
    local_dir = "/Users/ytong/temp/fss_output"
    try:
        shutil.rmtree(local_dir)
    except FileNotFoundError:
        ...

    http_fs = fsspec.filesystem("http")

    http_signed_url = longUrl
    http_fs.get(http_signed_url, os.path.join(local_dir, "from_http"))

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@cgrass cgrass added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Jan 9, 2024
Copy link

dosubot bot commented Jan 9, 2024

🤖

Hey @cgrass! 👋 Just a quick heads up: I'm Dosu, a friendly bot here to lend a helping hand while we wait for a human maintainer to assist you. I'm equipped to help you tackle bugs, answer queries, and guide you on your journey to becoming a contributor. So, don't hesitate to reach out if you need anything. Let's crack this code together! 🐞🔍

The issue you're encountering with Flytekit failing with long presigned URLs could potentially be resolved by implementing a mechanism to handle large payloads. This could involve splitting the payload into smaller chunks that are within the maxSizeInBytes limit, and then uploading these chunks separately.

In the context of the provided code, this would involve modifying the Get function in the AWSRemoteURL struct. Currently, this function retrieves the entire object from S3 and generates a presigned URL for it. Instead, it could be modified to retrieve and generate presigned URLs for smaller chunks of the object. However, this would also require changes on the client side (FlyteRemote) to handle the multiple URLs and reassemble the chunks into the original object. You can refer to the AWSRemoteURL.go file for more details.

As for the exact file system limit for the length of URLs in the Flytekit, I wasn't able to find a specific answer within the repository. The limit can vary depending on the file system and the operating system you're using.

Please note that this is a high-level suggestion and the actual implementation would depend on the specifics of your application and infrastructure. If you can provide more specific details about your setup and the exact error messages you're encountering, it would be helpful in providing a more detailed solution.

Sources


This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@eapolinario eapolinario added backlogged For internal use. Reserved for contributor team workflow. and removed untriaged This issues has not yet been looked at by the Maintainers labels Jan 18, 2024
@cgrass
Copy link
Contributor Author

cgrass commented Jan 24, 2024

@wild-endeavor - I got some feedback from the fsspec people on this bug. I don't expect them to fix it on their end anytime in the near future but they offered two workarounds:

  1. turn rpath and lpath into a list (you already saw this option)
  2. use get_file() instead of get()

I tested the second option and it seems to work, at least in the narrow use case exemplified by the test below. if lpath is a dir instead of a file the test fails.

def test_http_output():
    kwargs = {}
    fs = fsspec.implementations.http.HTTPFileSystem(fsspec.filesystem("https", **kwargs))
    expected_output_path = "/var/myloc/input"
    rpath = {longS3SignedUrl}
    lpath = expected_output_path
    fs.get_file(rpath, lpath)

@wild-endeavor
Copy link
Contributor

ah gotcha thank you for letting me know. I'm gonna get to this towards the end of this week. this code is kinda embedded in some other calls though - I might just rip this out for now. we might have a better solution in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants