-
Notifications
You must be signed in to change notification settings - Fork 443
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
feat: use uv as a pip alternative #1088
base: main
Are you sure you want to change the base?
Conversation
Hey Moises, these figures are getting everyone excited :) Do you think you'll be able to take this PR out of draft state? Or do you need some review? |
Hi @regisb, sorry for leaving this abandoned. I can work on this, but first I wanted to know people's opinion about the tool. IMO the tool is pretty awesome and slots in almost perfectly, the same applies to the other tool developed by the same team If the overall feeling is that it's worth a shot, I can clean up the PR and have it ready for review. |
Yes, ruff is awesome, to the point that I'm considering migrating the entire code base from pylint/black to ruff. But let's talk about this another day. For now I'd be curious to see this PR brought to completion. |
@MoisesGSalas What are the plans for this? We can target this for nightly so that it becomes part of sumac cutoff next week. |
373c066
to
0fca25f
Compare
@DawoudSheraz, @regisb, sorry I couldn't find the time to retake this. I think is ready for review. I wasn't quite sure what mechanism to use to fallback to the standard I rebased on top of nightly and targeted that branch. Build the image and imported a demo course just to check the platform launched correctly. I also mounted plugin using the |
@@ -51,8 +55,10 @@ RUN git config --global user.email "[email protected]" \ | |||
{{ patch("openedx-dockerfile-git-patches-default") }} | |||
{%- elif EDX_PLATFORM_VERSION == "master" %} | |||
# Patches in nightly node | |||
RUN curl -fsSL https://github.com/openedx/edx-platform/commit/3642cab3ac61ddfd360e7ceb7463b52be8c4deb0.patch | git am |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this patch needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edx-proctoring-proctortrack is installed as an editable VCS dependency but uv doesn't support that. I have this PR openedx/edx-platform#35654 with the same change that is used in that patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that uv doesn't support editable dependencies is a blocking issue, I think. I suggest we wait until the upstream issue is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are not going to implement it in the near future. Note that it refers to editable remote requirements, it works fine with editable requirements in a local path.
AFAIK the only use for an editable remote requirement is that it copies all the files, but you can fix that using the MANIFEST.in
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. The, using uv
means we will no longer be able to support adding editable dependencies to edx-platform. I'm not sure I'm ready to make that breaking change, as I expect that many people (Axim included) might add editable dependencies there.
Can we consider making an upstream PR to support that feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment gave me the impression they don't want to maintain code related to this feature:
As of now we intentionally don't support this. It adds a lot of complexity and we actually don't re-clone at all upon re-running the install command (we have a cache that contains a copy of each repository, and we checkout-and-install as needed with incremental fetches).
Maybe we can keep standard pip
as the default with the possibility to swap to uv
if your fork of edx-platform doesn't contain any editable dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can keep standard pip as the default with the possibility to swap to uv if your fork of edx-platform doesn't contain any editable dependency.
That's already the case. People can install uv in their own Docker image and alias pip='uv pip'
, all with a Tutor plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that wouldn't work for the RUN commands in the Dockerfile, right? The main appeal would be to use it in the python-requirements
layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that wouldn't work for the RUN commands in the Dockerfile, right?
It would. Install uv with the "openedx-dockerfile-minimal" patch, then ln -s /usr/bin/uv /usr/bin/pip
. Maybe this will be overwritten in the python-requirements layer, in which case we would have to introduce a new patch statement just above pip install -r /openedx/edx-platform/requirements/edx/base.txt
. Would you like to introduce such a patch?
uv
is a new package installer/resolver written in rust by the same people that wroteruff
.This is a small test to assess the viability of using
uv
instead of pip. It presents it self as a drop in replacement for pip, and for the most part it is, I only had to apply a single change due to limitations in editable VCS packages.I included the change in the edx-platform requirements as a patch, but I think it makes sense to upstream it, I don't think there's any reason to use an editable VCS requirement for proctortrack (there's even the discussion about droping it from the requirements altogether).
I used a build argument to make it possible to use pip instead of uv in case people have incompatible requirements in one of their forks/patches.
Tests
Using
--target=python-requirements
:--no-cache
uv
pip
uv
pip
During testing I added a
RUN touch /tmp/1
instruction right before thepip install -r /openedx/edx-platform/requirements/edx/base.txt
command to bust layer cache.