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: use alternate scheme for importing multipart #168

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

henryiii
Copy link
Contributor

This is an alternate attempt at #166 that allows a user to install the package and import it in a single Python process (not recommended, but Google Colab basically requires this to install anything).

This has the following pro's:

  • Doesn't rely on the import system loading a .pth file, meaning it can be hot swapped
  • Editable installs are supported (ironically, editable installs use the same .pth and custom loader that the original attempt did, but Google Colab users don't do editable installs!)
  • Less invasive on import multipart if multipart is not installed.

And the following con's:

  • Only handles filesystems; if the imports are not from file systems, it will not work (it will have the old collision behavior in that case).
    • This could be fixed if it's a problem, I think, but I think this is fine unless you have users loading both packages from a non-filesystem (like a zip).
  • A bit more invasive on multipart, specifically requiring that package not be moved to a module structure while this workaround is in place.

I also added a wrapper script for nox like the other scripts in /scripts.

@Kludex
Copy link
Owner

Kludex commented Oct 21, 2024

@defnull you good with the last point on the cons?

@defnull
Copy link
Contributor

defnull commented Oct 22, 2024

I have no plans to turn multipart.py into a package anytime soon. Since this is not a permanent thing but just a temporary workaround, I'm absolutely fine with that :)

@defnull
Copy link
Contributor

defnull commented Oct 22, 2024

A quick test looks promising:

$ python3 -mvenv venv
$ venv/bin/pip install git+https://github.com/henryiii/python-multipart.git@henryiii/fix/importer
[...] Successfully installed python-multipart-0.0.13

$ venv/bin/python -c "import multipart; print(multipart.__file__)"
<string>:1: FutureWarning: Please use `import python_multipart` instead.
/home/.../venv/lib/python3.12/site-packages/multipart/__init__.py

$ venv/bin/python -c "import python_multipart as multipart; print(multipart.__file__)"
/home/.../venv/lib/python3.12/site-packages/python_multipart/__init__.py

$ venv/bin/pip install multipart
[...] Successfully installed multipart-1.1.0

$ venv/bin/python -c "import python_multipart as multipart; print(multipart.__file__)"
/home/.../venv/lib/python3.12/site-packages/python_multipart/__init__.py

$ venv/bin/python -c "import multipart; print(multipart.__file__)"
/home/.../venv/lib/python3.12/site-packages/multipart.py

@Kludex
Copy link
Owner

Kludex commented Oct 22, 2024

I'll get a release with this when I have time.

@defnull
Copy link
Contributor

defnull commented Oct 22, 2024

This also works as expected in Google Collab:
image

The last edge case "conflict while importing from zip-files or other non-file-system imports" should be exceptionally rare and the important parts of this PR (being backwards compatible and printing a warning) should still work, so it's still an improvement over the current state.

@merwok
Copy link

merwok commented Oct 22, 2024

Is this rework needed? The original problem came from installing a new module and trying to import it in the same process without invalidating importlib caches

@henryiii
Copy link
Contributor Author

Colab users don't know they need to invalidate importlib caches, and the help in colab doesn't tell them they need to. And invalidating the import caches doesn't work, since the problem is the .pth file is run at startup, and is not related to import caches. Remember, the problem can be seen with:

!pip install python-multipart==0.0.13
import multipart

The .pth files are run when the notebook kernel starts, which doesn't have any dependencies at all (Colab doesn't give you a way to pre-install deps). Then you install the package (which adds the .pth file), but it never gets a chance to run. Adding an importlib.invalidate_caches() in-between has no effect.

FWIW, this works, because it does exactly what the .pth file would have done:

!pip install python-multipart==0.0.13
import _python_multipart_loader
import multipart

But no one would know to do that.

@henryiii
Copy link
Contributor Author

If you restart the session, Colab does keep the environment so it works the second time. Also, this works:

!pip install python-multipart==0.0.13
import site
site.main()
import multipart

But again, no one would know they need to do that.

scripts/README.md Outdated Show resolved Hide resolved
@Kludex
Copy link
Owner

Kludex commented Oct 24, 2024

Thanks. Let's try this.

@Kludex Kludex merged commit 0c04f4e into Kludex:master Oct 24, 2024
6 checks passed
@witt3rd
Copy link

witt3rd commented Oct 24, 2024

I am using rye (uv) and not pip and now FastAPI is broken since it can't import multipart.

@Kludex
Copy link
Owner

Kludex commented Oct 24, 2024

Yeah... I also can't import... 🤔

@defnull
Copy link
Contributor

defnull commented Oct 24, 2024

The build artefacts uploaded to pypi do not contain the second package for some reason. Installing directly from git also does not work, but installing from git+https://github.com/henryiii/python-multipart.git@henryiii/fix/importer does in fact still work. What!?

@Kludex
Copy link
Owner

Kludex commented Oct 24, 2024

I'm yanking python-multipart 0.0.14 and Starlette 0.41.1.

@defnull
Copy link
Contributor

defnull commented Oct 24, 2024

Ahh, found the issue. Installing from source (via pip install git+https://github.com/Kludex/python-multipart.git) does in fact work, but the source and wheel distribution files uploaded to pypi are incomplete. They are missing the /multipart/* files, and that's because in pyproject.toml the [tool.hatch.build.targets.sdist] section is still from the previous attempt. Wheels are built from the source distribution, so missing files in the sdist file are also missing in the wheel.

@henryiii
Copy link
Contributor Author

Ah, forgot to rerun check-sdist. Sorry, will fix!

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.

5 participants