Skip to content

Conversation

frfahim
Copy link
Contributor

@frfahim frfahim commented Oct 27, 2024

Change Summary

This pull request is replacing the pdm package manager with uv in the CI workflows and other related configurations. The changes ensure that the new package manager is correctly set up and used across various jobs and scripts.
Makefile: Replaced pdm commands with uv for tasks such as installing dependencies, running tests, and generating documentation.

Related issue number

Fix: #10637

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Oct 27, 2024
Copy link

codspeed-hq bot commented Oct 27, 2024

CodSpeed Performance Report

Merging #10727 will not alter performance

Comparing frfahim:pdm-to-uv (7a654d2) with main (71c087c)

Summary

✅ 44 untouched benchmarks

Copy link
Contributor

github-actions bot commented Oct 29, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Looking great, thanks so much for your help here!

I've got a few questions / change requests!

@pydantic-hooky pydantic-hooky bot added the awaiting author revision awaiting changes from the PR author label Oct 31, 2024
@sydney-runkle
Copy link
Contributor

Very odd that we're seeing benchmark changes here...

@Viicos
Copy link
Member

Viicos commented Oct 31, 2024

Very odd that we're seeing benchmark changes here...

Benchmarks seem to be broken as they did not run and thus codspeed is for some reason reporting non sense results

@sydney-runkle
Copy link
Contributor

Maybe bumping to v3.0.0 will help...

@frfahim
Copy link
Contributor Author

frfahim commented Nov 2, 2024

Very odd that we're seeing benchmark changes here...

I don't have any clue why benchmarks are changing here, I was looking for a fix.

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Fixing the codspeed issue, hopefully :)

@frfahim
Copy link
Contributor Author

frfahim commented Nov 5, 2024

There is a conflict with pdm lock file, should I rebase it?
btw, CI's are not running.

@sydney-runkle
Copy link
Contributor

All good, I can take it from here. Thanks!

run: |
pdm venv create --with-pip --force $PYTHON
pdm install -G docs
run: uv sync --python 3.12 --group docs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

uv automatically checks requires-python constraints and use that python version. Is it necessary to mention the python version again here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem redundant...

Copy link
Member

Choose a reason for hiding this comment

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

So the thing is uv will prefer the specified version in the .python-version file if present, no matter the previously installed version. We currently don't have such a file, but it could be pretty bad if we end up creating one at some point, especially for the jobs with a Python version matrix: CI will only run on the version from .python-version, and we won't notice anything.

It's a bit unfortunate, probably using tox (and tox-uv) could help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when a python version matrix exists, then python version is specified with the sync command
uv sync --python ${{ matrix.python-version }}

So it will use the requested python version from matrix even the .python-version file exist

sydney-runkle and others added 3 commits November 5, 2024 17:32
Co-authored-by: Farhadur Reza Fahim <farhadur.fahim@gmail.com>
Co-authored-by: Farhadur Reza Fahim <farhadur.fahim@gmail.com>
Co-authored-by: Farhadur Reza Fahim <farhadur.fahim@gmail.com>
@Viicos
Copy link
Member

Viicos commented Nov 7, 2024

Seems like in some CI jobs, we run uv install ... and then uv sync ..., and in others we only run uv sync .... uv sync should call uv install right? Maybe it's best to be explicit and have a uv install ... step for every job

@frfahim
Copy link
Contributor Author

frfahim commented Nov 7, 2024

Seems like in some CI jobs, we run uv install ... and then uv sync ...

uv sync command is used everywhere, there is no use of uv install command in the CI's

@sydney-runkle sydney-runkle added relnotes-packaging Used for dependency changes. and removed awaiting author revision awaiting changes from the PR author relnotes-fix Used for bugfixes. labels Nov 7, 2024
@sydney-runkle
Copy link
Contributor

I've removed the unnecessary installs :)

@sydney-runkle
Copy link
Contributor

Unclear to me why that test is failing... investigating now.

@sydney-runkle
Copy link
Contributor

@frfahim,

Any chance you could take a look here?

@frfahim
Copy link
Contributor Author

frfahim commented Nov 7, 2024

Sure, investigating the issue 👀

@sydney-runkle
Copy link
Contributor

Looks like we're installing an older version of Python 3.9 - not sure why - previously, it was 3.9.20, now it's 3.9.6 without the manual install before the sync.

@frfahim
Copy link
Contributor Author

frfahim commented Nov 7, 2024

It's using system python version, for macos 13 (with 3.9) it is 3.9.6

Run uv sync --python 3.9 --group testing --extra timezone
Using CPython 3.9.6 interpreter at: /Applications/Xcode_15.2.app/Contents/Developer/usr/bin/python3

It looks like it's not using uv installed python version for macos 13 with 3.9

This command will fix the issue, but I am looking for a better solution
uv sync --python ${{ matrix.python-version }} --python-preference only-managed

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Thanks so much!! Great work!

@sydney-runkle sydney-runkle merged commit 53bf2f2 into pydantic:main Nov 8, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-packaging Used for dependency changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to uv instead of pdm
3 participants