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

Better Python provider #1128

Open
CoderJoshDK opened this issue Jun 27, 2024 · 0 comments
Open

Better Python provider #1128

CoderJoshDK opened this issue Jun 27, 2024 · 0 comments

Comments

@CoderJoshDK
Copy link

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.

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.

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?

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

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.

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

No branches or pull requests

1 participant