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

Drop python-multipart dependency #2390

Open
Kludex opened this issue Dec 29, 2023 · 12 comments
Open

Drop python-multipart dependency #2390

Kludex opened this issue Dec 29, 2023 · 12 comments
Labels
feature New feature or request
Milestone

Comments

@Kludex
Copy link
Member

Kludex commented Dec 29, 2023

We should have the multipart parsing on Starlette itself.

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@Kludex Kludex added the feature New feature or request label Dec 29, 2023
@Kludex Kludex modified the milestones: Version 1.0, Version 1.x Feb 17, 2024
@nanda-mik
Copy link

@Kludex, I'd like to take on this task, please assign me. I've understood the parser classes and written test cases for them. I've also started implementing the parser functionality to phase out the dependency on python-multipart. I will try to complete this in some days and will submit a PR then.

Could you provide any specific things or best practices I should follow during the implementation? Additionally, would it be preferable to push the entire change at once or make smaller, incremental PRs?

Thanks!

@rafalkrupinski
Copy link

what's wrong with python-multipart?

@Kludex
Copy link
Member Author

Kludex commented Aug 11, 2024

Nothing.

@adriangb
Copy link
Member

I think the point is that even if nothing is wrong with it Starlette should still absorb that functionality since it's quite core to what Starlette does.

@rafalkrupinski
Copy link

I'd rather extract functions like this from existing projects, for greater modularization and to facilitate re-use, but you be you.

@defnull
Copy link

defnull commented Sep 20, 2024

You could also consider switching from python-multipart to the (significantly smaller and faster) multipart project, or even vendor it if you prefer. The MIT license would allow shipping multipart.py as part of starlette.

Disclaimer: I'm the author of multipart and recently learned that the python-multipart project causes name conflicts for people because they decided to choose a package name that was already taken. I took that as an opportunity to clean up the project, release a new non-blocking parser that I was working on for a while now, increase test coverage to 100% and today I finally published the 1.0 release. I would obviously love to see starlette depend on multipart instead of python-multipart, but I'm probably biased ;)

Edit: I learned after writing this comment that python-multipart and starlette are developed by the same people, so switching to a different implementation is probably not an option for non-technical reasons.

@Kludex
Copy link
Member Author

Kludex commented Oct 11, 2024

Edit: I learned after writing this comment that python-multipart and starlette are developed by the same people, so switching to a different implementation is probably not an option for non-technical reasons.

I mean, the idea here was to first make python-multipart modern (type hints + 100% coverage), so we could merge the parts Starlette use to Starlette itself. But I didn't want to introduce bugs in the way, so the easiest way is just to vendor the package that is already being used.

I'm open to try to make that part of the code customizable in case people want to try custom parsers.

@Kludex
Copy link
Member Author

Kludex commented Oct 11, 2024

As a curiosity, there was a PR long ago to replace python-multipart by another parser: #1514

@defnull
Copy link

defnull commented Oct 11, 2024

I'm open to try to make that part of the code customizable in case people want to try custom parsers.

Not sure if that's really necessary. I originally suggested a switch because of the import name conflict between multipart and python-multipart (discussed in other issues, off-topic here), but merging python-multipart into Starlette and no longer installing it as a dependency would also solve the dependency conflict for most users. Once python-multipart is merged, I do not really see the need for a plug-able parser interface in Starlette.

As a curiosity, there was a PR long ago to replace python-multipart by another parser: #1514

Oh fun, I was first exited to learn about a new parser to benchmark, but it looks like baize just slightly modified the non-blocking parser from werkzeug (without giving credit).

@tomchristie
Copy link
Member

You could also consider switching from python-multipart to the (significantly smaller and faster) multipart project.

That seems like a really decent idea.
(Given the comprehensibility of the code, attention to maintainance & results of benchmarks.)

I learned after writing this comment that python-multipart and starlette are developed by the same people, so switching to a different implementation is probably not an option for non-technical reasons.

I'm not sure that's how we tend to operate round these parts? 😊

I'd be interested to see a draft pull request switching MultiPartParser to using multipart for it's implementation.

@aguckenber-chwy
Copy link

Related partially but if not dropped, can the import be changed?
image

@Kludex
Copy link
Member Author

Kludex commented Nov 5, 2024

It's changed already. Bump Starlette.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants