-
Notifications
You must be signed in to change notification settings - Fork 239
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
Better Python provider #1128
Comments
Another example of My goal with introducing |
I am very busy right now since I recently got a new job. But I am coming back here to update about uv. They now have a lock file. So we can ignore the whole "use uv as default" and just use it if that file is detected. I still think using it as the default has its advantages, but I do also understand the disadvantages; and so looking for the |
Thanks for all these suggestions. Much appreciated and contributions to the Python provider are very welcome. I'll try and address everything, but let me know if anything is missed.
For the most part, yes. But there are a few very similar examples that are only tested with the snapshot tests. These simply test what build plan is generated. However, for all the main examples (like Python Django) we should ideally be testing that everything starts and runs as expected. It looks like an oversight for the Python Django example.
Yes 100%
Also yes Re UVI think using UV if a |
agree, following what the project does is most desirable for expected results |
Feature request
There are a few minor things I have noticed about the python provider. Some of these things should be fixed / updated. Some are pure opinionated ideas. This is all going to be tracked under 1 issue.
Nits
This first thing is super minor.
nixpacks/src/providers/python.rs
Lines 192 to 194 in 9fae921
This line includes
pip install --upgrade build setuptools
. However, it can be removed. Strictly speaking, it doesn't actually do anything. The install phase is an installation, not a build. There is no use of any build tools here. And if a build tool is required for thepip install .
, it should be properly configured in thepyproject
. IE,pip
will just handle it, itself.Removing this line is a minor optimization.
Testing inconstancy
In the process of local testing what would happen if I removed the setuptools install, I noticed a few inconsistent things with testing. For starters, the file,
examples/python-setuptools
, is never tested indocker_run_tests
. Shouldn't everyexample
be tested?I also noticed that
examples/python-django
isn't actually a valid example. Gunicorn isn't included in therequirements
file.nixpacks/examples/python-django/requirements.txt
Lines 1 to 4 in 9fae921
I am of the opinion that all examples should be at least valid, even if not useful. Functionally, this ends up not making a huge difference. Tests still pass because they don't check the status of the start phase. Running a server would hang the image; however, there is already logic to kill any image running for more than 20 seconds. So this isn't a real issue. The question just becomes, should tests be more consistent? I say that, because other providers also have hanging servers, and they are allowed to properly build and hang. So I hold that python examples should also do the same. (big + is that all examples are valid)
Side question
Not really part of the main discussion. But should tests make sure that the start phase is actually able to start? This feels like a worthy thing to test.
Opinions
These are much more opinionated. I might be making a few wrong assumptions.
Why are
venv
files being copied to/opt/venv
instead of just using its relative path?nixpacks/src/providers/python.rs
Line 154 in 9fae921
The whole idea of
--copies {env_loc}
feels unnecessary to me. Couldn't it, and shouldn't it, just be the default relative path? This avoids the need for any--copies
. The only reason I can think of to not do that, is if you wanted to allow the caller to modify this location. But that isn't an option anyway.The only other thing that this might be trying to solve, is
install_phase.add_path(format!("{env_loc}/bin"));
? Does nixpacks not play nice with the relative pathing? I would think this doesn't matter, as it should be deterministic. But I am obviously not a nixpacks master.UV
The default package manager should be UV. Not for package management, but for installing. UV is multitudes faster than
pip
. The main motivation behind this, is faster build times.pip
is that slow. This example is a little extreme. But only a little.UV is already a nix package. All that would be required, is adding
uv
tonixpacks/src/providers/python.rs
Line 148 in 9fae921
And then replacing
pip ...
withuv pip ...
. This has been tested on everything butpython2
, and works well. However, I am missing benchmarks. I will only bother with benchmarks if this change is favored positively. Of course, we can ignoreuv
when some other package manager is specified. But by having a more performant default, this will save users and platforms time. Read the pip compatibility readme to get an idea of the full picture. But tl;dr, the only thing it doesn't do (in this use case), is handle pip global environment variables.It is opinionated. But it isn't like there are no opinions already in the system. They are minor opinions, sure. But the provider is not purely clear of any opinions.
Motivation
Trying to help improve the tools I can. Ultimately, this is me trying to get more experience with dev tools and rust. I end up just using docker for my builds / deploys. Regardless, I hope to be able to help improve things.
Contribution
After deciding what should and should not change, I can make PRs for all of them. And if I get stuck on how to implement something, I can either discuss here (or discord) or open a new issue.
The text was updated successfully, but these errors were encountered: