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

construct url-safe url when downloading file onto container #453

Merged
merged 13 commits into from
May 2, 2024

Conversation

plutopulp
Copy link
Contributor

@plutopulp plutopulp commented May 1, 2024

When downloading a file onto the container, we safe-encode the url before downloading the file, e.g. white spaces and other special chars will be encoded etc..
Also, to ensure backward compatibility (for pipelines using older pipeline-ai versions) and to encode any file_urls at the API entry level, we update the RunInput schema to recursively encode any file_url nested within. Note also we ensure that urls are not encoded multiple times.

Copy link

linear bot commented May 1, 2024

@plutopulp plutopulp marked this pull request as ready for review May 1, 2024 15:12
Copy link
Contributor

@MPCherry MPCherry left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@rossgray rossgray left a comment

Choose a reason for hiding this comment

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

Looks sensible to me. I have to admit, I don't fully understand the whole flow here but what you've done looks correct and like the fact that you've added tests to cover all the different permutations. I wasn't sure whether calling encode_url which has a pydantic decorator on it would work outside of pydantic validation but it obviously must do :)

Thanks for fixing this 👍

@plutopulp
Copy link
Contributor Author

Looks sensible to me. I have to admit, I don't fully understand the whole flow here but what you've done looks correct and like the fact that you've added tests to cover all the different permutations. I wasn't sure whether calling encode_url which has a pydantic decorator on it would work outside of pydantic validation but it obviously must do :)

Thanks for fixing this 👍

Yeah no worries 👍. I wasn't sure either about calling the pydantic decorated method from outside either, but it worked. Considered moving the encoding logic to a utility function defined outside the pydantic model, but probably fine as is.

@plutopulp plutopulp merged commit fcc8480 into main May 2, 2024
5 checks passed
@plutopulp plutopulp deleted the plutopulp/pc-1104-files-with-spaces-fail branch May 2, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants