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

Fix: Support files > 1MB #169

Merged
merged 1 commit into from
Jul 11, 2024
Merged

Fix: Support files > 1MB #169

merged 1 commit into from
Jul 11, 2024

Conversation

andyland
Copy link
Contributor

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

This PR allows files that are larger than 1MB to be uploaded.

For files larger than 1MB uploaded with FastAPI, it creates a SpooledTemporaryFile which is partially written to disk as described here. This prevents pickling of the form by the multiprocess library and crashes the request.

This PR sets the limit to an extremely large number so the file is always kept entirely in memory. This allows the file to be serialized correctly. Effectively, this limits the max file size for LitServe to the amount of RAM available on the machine.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78%. Comparing base (450bc08) to head (61260c5).

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #169   +/-   ##
===================================
  Coverage    78%    78%           
===================================
  Files        13     13           
  Lines       946    949    +3     
===================================
+ Hits        734    737    +3     
  Misses      212    212           

@andyland andyland enabled auto-merge (squash) July 11, 2024 07:09
Copy link
Collaborator

@aniketmaurya aniketmaurya left a comment

Choose a reason for hiding this comment

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

LGTM 🔥 . Thanks @andyland!

@andyland andyland merged commit df5ae3e into main Jul 11, 2024
19 checks passed
@andyland andyland deleted the andy/fix-file-size branch July 11, 2024 08:23
@lantiga
Copy link
Collaborator

lantiga commented Jul 11, 2024

about this, we should have the ability to reject large uploads (max size configurable), or a ill intentioned consumer of the api could upload a huge file and crash the server

@andyland
Copy link
Contributor Author

@lantiga Yeah, this PR works around a technical limitation preventing us from supporting larger requests. For that, I would recommend a different approach. I'd attach a middleware to FastAPI to return a proper 413 HTTP code on some configurable size.

@lantiga
Copy link
Collaborator

lantiga commented Jul 12, 2024

yes definitely, that’s also what I was thinking above
@aniketmaurya can you add an issue to track it?

@aniketmaurya
Copy link
Collaborator

created an issue here #171

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