Skip to content

use uv as venv/dependency orchestrator #17978

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

Merged
merged 17 commits into from
Apr 28, 2025
Merged

use uv as venv/dependency orchestrator #17978

merged 17 commits into from
Apr 28, 2025

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Apr 16, 2025

Link to issue number:

Depends on nvaccess/nvda-misc-deps#36
closes #17935 

Summary of the issue:

We're currently handling the NVDA virtual environment with custom scripts. Furthermore, we use requirements.txt as our only locking mechanism.

Description of user facing changes

  1. For developers, blazing fast venv generation and venv spinning up and down. Also much more ease when dependencies are updated or added. On github actions, build times are improved by around two minutes when there is a cache for uv.lock.
  2. improvement of add-on development workflow when uv is used there as well. When uv is adopted, I will propose an update for the add-on template.

Description of development approach

  1. Installed uv
  2. Adapted pyproject.toml to contain everything from requirements.txt in several dependency groups
  3. removed venvutils as no longer needed. Just use uv run
  4. Adapted the several batch scripts to use uv run
  5. Adapted sconstruct and nvda.pyw to ensure that an uv managed venv is used and that it is the environment belonging to the project.
  6. Adapted the github workflow to use the uv setup action. Might want to pin this to a particular version
  7. created ensureunv.bat that is currently used inside scons.bat to provide a simple uv installation experience. It offers to install uv using winget (recommended IMO) and using the official install script. The latter stores uf in %userprofile%/.local/bin
  8. removed sourceEnv module as this workaround is no longer necessary now miscDeps is a workspace member

Testing strategy:

Test running from source as well as basic tests of build artifacts.
Ensured that switching from master to my uv branch works, i.e. it is not necessary to rebuild the venv though I think it is encouraged to do so.

Known issues with pull request:

I initially considered switching to the uv provided python builds, i.e. https://github.com/astral-sh/python-build-standalone/
However, there are several show stoppers with that:

  1. The test module is missing from the python builds, therefore the unit tests fail as they contain a mocker (notably, nvda remote transport tests). That said, the test module is not meant to be used by other projects.
  2. These builds differ from normal python builds, see https://gregoryszorc.com/docs/python-build-standalone/main/quirks.html#windows-static-distributions-are-extremely-brittle

We probably want to handle this in a follow up. At first sight, apart from the tests, things mostly worked as normal.

I was also unable to handle the constraint in constraints.txt as uv doesn't seem to understand it. That said, regardless whether I'm using the constraint or not, there seems to be a wheel for pillow, so I don't see why this constraint is still necessary.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@LeonarddeR LeonarddeR requested a review from a team as a code owner April 16, 2025 20:38
@LeonarddeR LeonarddeR requested a review from SaschaCowley April 16, 2025 20:38
@LeonarddeR
Copy link
Collaborator Author

note that while the diff is huge, uv.lock is a very huge file.

@LeonarddeR
Copy link
Collaborator Author

Last run of Prepare source code on master: 7m 19s
Last run of Prepare source code in this pr using uv: 6m 6s

@bramd
Copy link
Contributor

bramd commented Apr 16, 2025

@LeonarddeR Regarding the constraint on Pillow, uv does support the -C option to pass on options to the builder, but those are not saved in pyproject.toml or uv.lock. Besides this, even if I remove Pillow and re-add it with the -C options specified, it just gets the wheel and builds nothing. So yes, I think we no longer need this.

@AppVeyorBot
Copy link

  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 49f897d871

@seanbudd seanbudd marked this pull request as draft April 17, 2025 00:23
@seanbudd
Copy link
Member

Please mark this as ready when both AppVeyor and GH actions builds are succeeding. I want to be able to compare build times

@seanbudd seanbudd self-requested a review April 17, 2025 00:24
@seanbudd
Copy link
Member

I think it might be better to make uv.lock generated on GitHub actions. I don't know if we can trust community generated changes super easily. For this PR I would be generating the file myself and comparing the diff

@AppVeyorBot
Copy link

  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 1b0703ae9b

@AppVeyorBot
Copy link

  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 720cb37e15

@LeonarddeR
Copy link
Collaborator Author

I think it might be better to make uv.lock generated on GitHub actions. I don't know if we can trust community generated changes super easily. For this PR I would be generating the file myself and comparing the diff

The lock file is meant to go into version control. This is ultimately the way to get a consistent build experience. NO longer changing dependencies without being aware of it.
Do I understand you correctly that you believe uv.lock should be out of version control, or that it should be in version control but that community contributions should not change it? I guess that can be checked with pre-commit somehow.

@LeonarddeR
Copy link
Collaborator Author

unfortunately, the run nvda script leaves an empty window open. See astral-sh/uv#11786

@seanbudd
Copy link
Member

I think the lock file should be in version control, but generated and committed by a CI action. Or at the very least, verified against a CI built file when changed. Is this feasible and does it make sense for development?

@bramd
Copy link
Contributor

bramd commented Apr 18, 2025

@seanbudd As long as we keep version constraints in pyproject.toml I see the lock file as an additional safeguard to get the exact dependencies we expect. It gives us more granular control then the requirements.txt. We could choose to set wider version constraints in pyproject, for example comtypes==1.4.6 could become comtypes<1.5,>=1.4.6, stating that we expect things to work with any 1.4.x patch releases for comtypes. The exact resolved version will be recorded and locked in uv.lock and stay the same until someone updates the lock file using uv lock --update. Widening version constraints could also help us use things like Dependabot more reliably.

Running uv lock --check could be part of our CI to ensure the uv.lock matches what's in pyproject.toml. Besides that, I think checking any changes to uv.lock should be done as part of code review. Based on some small tests the uv.lock file format seems quite diff friendly.

@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: System tests (tags: installer NVDA).
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.3,
    INSTALL_END 1.0,
    BUILD_START 0.0,
    BUILD_END 20.6,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.4,
    TEST_START 0.0,
    TEST_END 19.5,
    FINISH_END 0.2

See test results for failed build of commit bf2589278c

@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: System tests (tags: installer NVDA).
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.3,
    INSTALL_END 0.9,
    BUILD_START 0.0,
    BUILD_END 19.8,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.4,
    TEST_START 0.0,
    TEST_END 19.6,
    FINISH_END 0.2

See test results for failed build of commit c38744f1a5

@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: System tests (tags: installer NVDA).
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.3,
    INSTALL_END 0.9,
    BUILD_START 0.0,
    BUILD_END 19.7,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.4,
    TEST_START 0.0,
    TEST_END 19.4,
    FINISH_END 0.2

See test results for failed build of commit 425e1eade6

@LeonarddeR LeonarddeR marked this pull request as ready for review April 18, 2025 20:28
@LeonarddeR LeonarddeR requested a review from a team as a code owner April 18, 2025 20:28
@LeonarddeR LeonarddeR requested a review from Qchristensen April 18, 2025 20:28
@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: System tests (tags: installer NVDA).
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.3,
    INSTALL_END 0.9,
    BUILD_START 0.0,
    BUILD_END 19.1,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.4,
    TEST_START 0.0,
    TEST_END 19.5,
    FINISH_END 0.3

See test results for failed build of commit e2c29b856f

@wmhn1872265132
Copy link
Contributor

Please note: The l10nUtil tool in NVDA launcher cannot run:

Traceback (most recent call last):
File "l10nUtil.py", line 6, in
ModuleNotFoundError: No module named 'crowdin_api'

@LeonarddeR
Copy link
Collaborator Author

Thanks @wmhn1872265132, should be fixed now.

@LeonarddeR LeonarddeR marked this pull request as ready for review April 22, 2025 14:32
@@ -20,7 +17,7 @@ if (!$env:APPVEYOR_PULL_REQUEST_NUMBER -and $env:versionType) {
}

# Save an exact list of the actual Python packages and their versions that got installed, to aide in reproducing a build
.\venvUtils\exportPackageList.bat installed_python_packages.txt
uv export -q > installed_python_packages.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing it makes sense

LeonarddeR and others added 2 commits April 24, 2025 07:32
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
seanbudd pushed a commit to nvaccess/nvda-misc-deps that referenced this pull request Apr 24, 2025
See nvaccess/nvda#17978
This ensures that python miscDeps become a python package that can be included as dependency in uv. Then it is no longer necessary to use a sourceEnv script.
@AppVeyorBot
Copy link

See test results for failed build of commit 4d04d567b0

@seanbudd
Copy link
Member

@LeonarddeR - the build is failing here

@seanbudd seanbudd marked this pull request as draft April 28, 2025 01:05
@LeonarddeR
Copy link
Collaborator Author

I'm ot sure whether the system test failures are related here, given they succeeded on gh Actions this time. May be there is a dependency difference that increases the likelyhood of failure, I'lll investigate.

@seanbudd
Copy link
Member

@LeonarddeR - my mistake - I thought GH was still failing. The chrome test failure should be fine to ignore

@seanbudd seanbudd marked this pull request as ready for review April 28, 2025 04:49
@seanbudd seanbudd merged commit 3616aef into nvaccess:master Apr 28, 2025
4 of 5 checks passed
@seanbudd
Copy link
Member

Thanks for this PR!

@CyrilleB79
Copy link
Contributor

@LeonarddeR I just realize that uv mention in projectDocs/dev/createDevEnvironment.md is very brief. Having to create a new environment, I have read it more carefully.

The doc reads:

#### uv
[uv](https://docs.astral.sh/uv/) is used as package and project manager.

and the paragraph is along with Python and MS Visual Studio among the dependencies to be installed. So I guess I should use the command line on the page linked to install it.

On the other side, the change log reads:

* NVDA now uses [uv](https://docs.astral.sh/uv/) as Python package/project manager. (#17935, @LeonarddeR)
  * Running `scons` from the source repository will automatically suggest a strategy to install uv when it is not yet available.

So should we rather expect to be suggested a strategy? Or maybe it was just intended for people already having an old environment installed?

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.

Investigate uv as venv/package manager
6 participants