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.

Sign up for GitHub

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

Jump to bottom

Better Python provider #1128

Open
CoderJoshDK opened this issue Jun 27, 2024 · 4 comments · May be fixed by #1216
Open

Better Python provider #1128

CoderJoshDK opened this issue Jun 27, 2024 · 4 comments · May be fixed by #1216

Comments

@CoderJoshDK
Copy link
Contributor

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

let mut install_phase = Phase::install(Some(format!(
"{create_env} && {activate_env} && pip install --upgrade build setuptools && pip install ."
)));

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 the pip install ., it should be properly configured in the pyproject. 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 in docker_run_tests. Shouldn't every example be tested?
I also noticed that examples/python-django isn't actually a valid example. Gunicorn isn't included in the requirements file.

nixpacks/examples/python-django/requirements.txt

Lines 1 to 4 in 9fae921

asgiref==3.8.1
Django==5.0.6
psycopg2-binary==2.9.9
sqlparse==0.5.0

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

let env_loc = "/opt/venv";

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 to

nixpacks/src/providers/python.rs

Line 148 in 9fae921

setup.add_nix_pkgs(&[Pkg::new("gcc")]);

And then replacing pip ... with uv pip .... This has been tested on everything but python2, and works well. However, I am missing benchmarks. I will only bother with benchmarks if this change is favored positively. Of course, we can ignore uv 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.
@CoderJoshDK
Copy link
Contributor Author

Another example of uv just being really good™️ : https://blog.streamlit.io/python-pip-vs-astral-uv/

My goal with introducing uv is to make things faster. At least on the default happy path. pip is just that slow.

@CoderJoshDK
Copy link
Contributor Author

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 uv.lock file is more than good enough.
I won't be able to get to this for ... a while. But if no one picks this change up, I do plan to eventually come back and add it. Shouldn't be hard.

@coffee-cup
Copy link
Contributor

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.

Shouldn't every example be tested?

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.

I am of the opinion that all examples should be at least valid, even if not useful

Yes 100%

But should tests make sure that the start phase is actually able to start?

Also yes

Re UV

I think using UV if a uv.lock file is found is a good idea. Uv may be faster, but Pip is still fairly standard in the ecosystem and changing the default package manager for Python is a fairly significant change. Any (even small) differences will be noticed. Projects that are using UV will likely have a uv.lock file already.

@JacobCoffee
Copy link

JacobCoffee commented Oct 9, 2024 •
edited
Loading

using UV if a uv.lock file is found is a good idea

agree, following what the project does is most desirable for expected results

@iloveitaly iloveitaly linked a pull request Nov 7, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: uv support, py 3.13 iloveitaly/nixpacks
3 participants
@coffee-cup @JacobCoffee @CoderJoshDK