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

refactor: rename import to python_multipart #166

Merged
merged 3 commits into from
Oct 20, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Oct 11, 2024

This renames the import to python_multipart, and installs a custom Finder that allows legacy code to continue to work unless the PyPI multipart package is installed, in which case the custom Finder no longer does anything. A warning is shown asking the user to import python_multipart instead of import multipart if they use this. This could be customized if you'd rather have it not show a warning at first (to give libraries time to update, but without a warning I don't know if they will know to update), or show a warning if multipart is installed saying "if you meant to use python_multipart, you are getting multipart instead since you have both installed" (but regular multipart users would unavoidably see that warning, so I don't think it's a great idea).

I don't think this will support editable installs (Hatchling's force_include doesn't support editable installs), but if someone is doing an editable install, they can use python_multipart.

I tested it using nox.

I think this will provide a smooth path forward for a painless rename, backward compatibility support for current users, and help users stuck and unable to install both packages in the long run. WDYT?

Cross-reference: #16 pypa/packaging-problems#818

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii changed the title refactor: rename to python_multipart refactor: rename import to python_multipart Oct 11, 2024
@defnull
Copy link
Contributor

defnull commented Oct 12, 2024

Wow, I knew this should be possible and played with the idea myself but was unsure how to actually tackle it. If this works, that would be extremely helpful to downstream developers. I'll test this myself as soon as I'll find the time. Very interesting solution. The pth file could be removed again after a couple of releases I guess?

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Contributor Author

Ahh, sorry, I did find and run scripts/check, I just thought it reformatted it for me!

@henryiii
Copy link
Contributor Author

If you remove the .pth file and the file it includes, then it goes back to being a normal library and import multipart would be gone, leaving only import python_multipart. So yes, it could be removed in the future if reasonably sure all the users have moved to the new import name.

@defnull
Copy link
Contributor

defnull commented Oct 12, 2024

So in my understanding it works like this:

  • If just python_multipart is installed, then import multipart will still work but print a warning. This is beautiful. A fully backwards compatible solution that also tells users how to move forward.
  • If both python_multipart and multipart are installed, then there is already a conflict situation. This should be rare. But if it happens, import multipart actually imports multipart. Users that need python_multipart can still import it with the new name, so there is a clean path forward.

If, for example, twisted decides to move from email to multipart, and a Starlette application has twisted as a dependency, currently this would silently break some functionality in twisted. After this change, it would just work. Given of cause that new releases of Starlette use the new import name.

The only situation where this still breaks is when someone depends on Starlette and Twisted and only updates twisted. That does not sound like a common scenario.

@henryiii
Copy link
Contributor Author

Yes, that's correct.

mgorny added a commit to mgorny/gentoo that referenced this pull request Oct 19, 2024
Hard-rename the Python package to `python_multipart`, to avoid conflicts
with the `multipart` PyPI package.

Both `multipart` and `python-multipart` packages install using
`multipart` import name, therefore both cannot be installed at the same
time.  Historically, we have included only the latter in Gentoo, since
it was pulled in as a dependency of dev-python/starlette.  However,
the former was recently made the recommended replacement
for the deprecated and removed `cgi` standard library module, therefore
other packages started depending on it.

Given that both packages intend to be maintained throughout
the foreseeable future, it seems that the best workaround is to rename
one of them.  In this case, `python-multipart` uses an import name
that does not match the package and seems to be have fewer reverse
dependencies at this time, so rename it.  There is already an open pull
request upstream to do exactly that, so reuse parts of it.  That said,
since we can simply patch reverse dependencies, we do not need
the compatibility layer (and probably do not want it, as it could yield
confusing errors if the wrong package is installed).

Bug: pypa/packaging-problems#818
Pull-Request: Kludex/python-multipart#166
Signed-off-by: Michał Górny <mgorny@gentoo.org>
mgorny added a commit to mgorny/gentoo that referenced this pull request Oct 19, 2024
Hard-rename the Python package to `python_multipart`, to avoid conflicts
with the `multipart` PyPI package.

Both `multipart` and `python-multipart` packages install using
`multipart` import name, therefore both cannot be installed at the same
time.  Historically, we have included only the latter in Gentoo, since
it was pulled in as a dependency of dev-python/starlette.  However,
the former was recently made the recommended replacement
for the deprecated and removed `cgi` standard library module, therefore
other packages started depending on it.

Given that both packages intend to be maintained throughout
the foreseeable future, it seems that the best workaround is to rename
one of them.  In this case, `python-multipart` uses an import name
that does not match the package and seems to be have fewer reverse
dependencies at this time, so rename it.  There is already an open pull
request upstream to do exactly that, so reuse parts of it.  That said,
since we can simply patch reverse dependencies, we do not need
the compatibility layer (and probably do not want it, as it could yield
confusing errors if the wrong package is installed).

Bug: pypa/packaging-problems#818
Pull-Request: Kludex/python-multipart#166
Signed-off-by: Michał Górny <mgorny@gentoo.org>
Copy link
Owner

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

I don't like that we are introducing another tool here (nox), but since it's your strong suit (and I don't want to work on this), I'll ignore it. 👀 👍

Thanks for this @henryiii 🙏

_python_multipart_loader.py Outdated Show resolved Hide resolved
@Kludex Kludex merged commit a629ae0 into Kludex:master Oct 20, 2024
6 checks passed
@LuisESV
Copy link

LuisESV commented Oct 20, 2024

Hey Hey! This rare case seems to be happening: "If both python_multipart and multipart are installed, then there is already a conflict situation. This should be rare."

I'm a beginner and I'm learning to code in python with the help of the bots (Claude, ChatGPT and phind) and they have suggested to install python_multipart to fix the issue of multipart module not found. I returned to 0.0.12 (after 3 hours of trying to understand haha) and now the error went away. This is what I was getting on Google Colab:

ModuleNotFoundError Traceback (most recent call last)
in <cell line: 1>()
----> 1 import gradio as gr
2 import random
3 import time
4 import requests
5 import base64

6 frames
/usr/local/lib/python3.10/dist-packages/gradio/route_utils.py in
32 import gradio_client.utils as client_utils
33 import httpx
---> 34 import multipart
35 from gradio_client.documentation import document
36 from multipart.multipart import parse_options_header

ModuleNotFoundError: No module named 'multipart'

@Kludex
Copy link
Owner

Kludex commented Oct 20, 2024

How can I reproduce it?

@henryiii
Copy link
Contributor Author

I can take a look later today! Is this just a basic colab notebook? Do you have both installed? You can’t use both packages at the same time, before or after this change.

@henryiii
Copy link
Contributor Author

henryiii commented Oct 20, 2024

Ahh, I think I know what’s happening. You are installing Python multipart after starting Python since you install it inside the running process. So that means the system path is already setup and the loader is not run.

image

@henryiii
Copy link
Contributor Author

henryiii commented Oct 20, 2024

@Kludex if you’d like to yank 0.0.13, I can see if I can provide a work around if the loader isn’t run tomorrow. I do have an idea to try. I’m assuming this is a large enough issue to worry about because it was caught very quickly. (Colab should not force users to do this, but with Google’s entire Python team fired, I’m not expecting any improvements here for a long time)

@Kludex
Copy link
Owner

Kludex commented Oct 20, 2024

Sure. I'll yank it. Thanks @henryiii 🙏

@Kludex
Copy link
Owner

Kludex commented Oct 20, 2024

I've yanked it. Thanks for helping here.

@merwok

This comment was marked as duplicate.

@mdierksen
Copy link

Form data requires "python-multipart" to be installed. 
You can install "python-multipart" with: 

pip install python-multipart

Traceback (most recent call last):
  File "/opt/netcon_scripts/fuzzy_api.py", line 987, in <module>
    @app.post("/check_spam")
     ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/fastapi/routing.py", line 994, in decorator
    self.add_api_route(
  File "/usr/local/lib/python3.12/site-packages/fastapi/routing.py", line 933, in add_api_route
    route = route_class(
            ^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/fastapi/routing.py", line 554, in __init__
    self.dependant = get_dependant(path=self.path_format, call=self.endpoint)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/fastapi/dependencies/utils.py", line 277, in get_dependant
    param_details = analyze_param(
                    ^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/fastapi/dependencies/utils.py", line 474, in analyze_param
    ensure_multipart_is_installed()
  File "/usr/local/lib/python3.12/site-packages/fastapi/dependencies/utils.py", line 107, in ensure_multipart_is_installed
    raise RuntimeError(multipart_not_installed_error) from None
RuntimeError: Form data requires "python-multipart" to be installed. 
You can install "python-multipart" with: 

pip install python-multipart

FastAPI is broken now.

fastapi           0.115.3
python-multipart  0.0.14

According to pip list no multipart module is installed.

/usr/local/lib/python3.12/site-packages/multipart/
└── __pycache__
    ├── decoders.cpython-312.opt-1.pyc
    ├── decoders.cpython-312.opt-2.pyc
    ├── exceptions.cpython-312.opt-1.pyc
    ├── exceptions.cpython-312.opt-2.pyc
    ├── __init__.cpython-312.opt-1.pyc
    ├── __init__.cpython-312.opt-2.pyc
    ├── multipart.cpython-312.opt-1.pyc
    └── multipart.cpython-312.opt-2.pyc

1 directory, 8 files


/usr/local/lib/python3.12/site-packages/python_multipart/
├── decoders.py
├── exceptions.py
├── __init__.py
├── multipart.py
├── __pycache__
│   ├── decoders.cpython-312.pyc
│   ├── exceptions.cpython-312.pyc
│   ├── __init__.cpython-312.pyc
│   └── multipart.cpython-312.pyc
└── py.typed

1 directory, 9 files

How do I fix this mess?

@Kludex
Copy link
Owner

Kludex commented Oct 25, 2024

No. It's not. I've yanked the release. Remove your environment and install the packages again.

@mdierksen
Copy link

Removing the installed modules and reinstalling them solved the issue. Thanks.

@henryiii
Copy link
Contributor Author

@Kludex, thanks for your patience working out the releases! I was wondering, would it be better to not include the warning on the next release, and only add it in a subsequent release? That way there would be less pressure to try to release other packages like Starlette immediately to avoid the warning.

@Kludex
Copy link
Owner

Kludex commented Oct 26, 2024

Hmmm... That would save me some energy. If you can do that, it would be nice @henryiii

Thanks 🙏

@henryiii
Copy link
Contributor Author

See #174.

@pawelad
Copy link

pawelad commented Oct 27, 2024

FWIW, the latest release still breaks FastAPI:

$ pip install python-multipart        
Collecting python-multipart
  Using cached python_multipart-0.0.15-py3-none-any.whl.metadata (1.8 kB)
Using cached python_multipart-0.0.15-py3-none-any.whl (24 kB)
Installing collected packages: python-multipart
Successfully installed python-multipart-0.0.15
$ gunicorn --pythonpath=src --worker-class=uvicorn.workers.UvicornWorker --reload foobar.app
...
RuntimeError: Form data requires "python-multipart" to be installed. 
You can install "python-multipart" with: 

pip install python-multipart

I 'fixed it' by pinning the version number to 0.0.12.

I feel like this change should be communicated to the FastAPI contributors / community, as they don't pin the correct version, and use some custom code to figure out if python-multipart is installed.

Related:

@Kludex
Copy link
Owner

Kludex commented Oct 27, 2024

I guess that's why import star is not recommended.

@Kludex
Copy link
Owner

Kludex commented Oct 27, 2024

I've fixed this on version 0.0.16.

@mdierksen
Copy link

@Kludex No offense, but if you are the 'FastAPI expert' then why you keep breaking it? You are even a member of the FastAPI team - one of only five members. You should be able to coordinate and test these changes before you release them. Please be more careful in the future.

@defnull
Copy link
Contributor

defnull commented Oct 28, 2024

@Kludex No offense, but if you are the 'FastAPI expert' then why you keep breaking it?

Renaming a package is tricky, there are lots of edge cases that are really hard to find during local testing and only show up in strange environments (Google Colab) or once you push a release (packaging bugs). Three (or more) people looked at those PRs and mistakes still happened. That's life. Broken releases were yanked immediately after problems were reported, so this should not have affected any production environments.

By the way, why does your project depend on a 0.0.x library without version-pinning? According to SemVer a 0.x version should not be considered stable and is allowed to break compatibility at any time. FastAPI does not depend on it directly, it's usually your own project that is pulling this dependency in and thus responsible for managing its version. If your CI/CD pipeline broke for a short while, than that's a good thing. Your automated tests worked as intended.

@Kludex
Copy link
Owner

Kludex commented Oct 28, 2024

Screenshot 2024-10-28 at 11 30 53

This is actually a good opportunity for me to write some of my thoughts, since I'm feeling sad about people on GitHub lately.

I maintain (pretty much alone) Starlette and Uvicorn, and by "maintain" I don't mean commit wise, which is also the case:

2 years of Starlette Screenshot 2024-10-28 at 11 36 13
2 years of Uvicorn Screenshot 2024-10-28 at 11 39 40

Most of the work actually comes by reviewing, replying discussions and triaging issues.

I took python-multipart over last year (the email is from 2022, but it was only transferred in 2023). Since it was unmaintained, and it's a dependency of Starlette, I thought it would be wise to ask for the PyPI and GitHub repository transfer.

Screenshot 2024-10-28 at 11 42 24

I don't have as much as sponsors as I could have, because I didn't create anything relevant. I just maintain others' people's packages. Last year, I got around 325 euros every month, where 300 dollars were from encode.

You may say: "oh, that's already more than a lot of other maintainer's receive"... Well... Considering my salary, and my qualifications, at this point, in an hour of consultancy I can get the same amount. 🤷‍♂️ Be aware that I'm likely one of the most active maintainers in the community. You can just see my history, or see my response time on issues if you doubt what I'm saying...

This year, I get a bit more. I talked to the FastAPI creator about how I feel, and Sebastián Ramírez, started sponsoring me 500 dollars a month. Which now I receive exactly $859.00 per month ($500 FastAPI, $300 encode, and $59 from 13 other sponsors).


About this issue on python-multipart... I push back hard mainly because I understand that sometimes there are unpredictable issues, and I didn't want to bring burden to Starlette/FastAPI users. I got convinced to go with this because many people complained to me on multiple issues downstream, not only on this repository... And some comments just dropped my energy:

Screenshot 2024-10-28 at 11 34 53

On the releases that happened here... I yanked them as fast I could. I may have done a poor review, probably because I respect @henryiii's work, and I may have been overconfident. 🤷‍♂️ I could have checked the wheel on build... 🤷‍♂️ I could have done a beta release?... 🤷‍♂️ I'm not sure if it matters. 🤷‍♂️ Easy to comment what to do after an incident happens.

Any reasonable project needs to have a lockfile. It doesn't matter if it's on ZeroVer, or SemVer, or PotatoVer. Maintainers break packages by mistake.

In any case, I've apologized...

Screenshot 2024-10-28 at 12 21 39

Multiple times...

Screenshot 2024-10-28 at 12 21 53

Wanna me to beg for forgiveness?

You are even a member of the FastAPI team - one of only five members.

Well... I understand that from the outside that's how things look, but I never said I'm one of the FastAPI maintainers. Otherwise I'd publicly say that, it benefits me. I do not maintain FastAPI. I have close contact with Sebastián, and I notify him when needed, but I don't maintain FastAPI. Obviously, I don't want to break FastAPI... I do what is within my range.


No offense [...]

Offense taken. I think you are being disrespectful.


In any case, I think this improvement makes sense. It's a backwards-compatibility solution. I'm happy we did it. 🙏

@mdierksen
Copy link

@Kludex Sorry, but I have no time reading all of that since I am currently busy building a CI/CD for that really tiny FastAPI project to make sure I catch broken releases of python-multipart in the future. Thank you for your time and effort. Enjoy your day, gentlemen.

@mgorny
Copy link

mgorny commented Oct 28, 2024

@Kludex No offense, but […]

Saying "no offense" before being an asshole doesn't make you any less of an asshole. In fact, it is a pretty clear indication that you realize you're being an asshole and you're trying to avoid responsibility for that.

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.

8 participants