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

chore: move to uv using just to run commands #174

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

miloth
Copy link

@miloth miloth commented Nov 16, 2024

Hi @jorenham , this aims at solving #49. The main changes are:

  • Package manager moved to uv.
  • Command runner changed to just.
  • tox and pre-commit use uv as venv backend.

It is a proof of concept, untested on GHA, hence the "draft" status. I just wanted to check if changing the command runner was an acceptable trade-off before continuing the work.

@jorenham jorenham marked this pull request as ready for review November 16, 2024 12:56
@jorenham
Copy link
Owner

Thanks a lot for this!
I wanted to see what the CI thought of it, so removed the draft status for a bit.

@jorenham
Copy link
Owner

What are the advantages for using just over poe?
FWIW; I believe that poe (unlike the name suggests) doesn't require poetry to work

@jorenham jorenham added the meta: packaging Building and release publishing label Nov 16, 2024
@jorenham
Copy link
Owner

FYI, I also tried migrating this to uv a while back:
https://github.com/jorenham/scipy-stubs/blob/uv/pyproject.toml

But for some reason I wasn't able to get it to work 🤔. My guess is that it has to do with either hatch or uv itself not supporting stub-only packages. And although I haven't verified this, I suspect this has to do with the - in the scipy-stubs package name, which for "normal" packages must be a valid python name.

@jorenham jorenham added meta: CI Continuous integration meta: deps Dependency management labels Nov 16, 2024
@miloth
Copy link
Author

miloth commented Nov 16, 2024

Yeah, I see the problem. I think it's related to how uv installs the packages in the venv. I came up with a workaround in the latest commit, basically forcing the install as a built wheel. Now the stubtest CI passes.

Going back to the choice between poe and just. I believe using Python as a command runner is a bit unnecessary. You are adding the complexity of an additional layer of interpreter for no added benefit. I prefer just, as it is a compiled tool, so much more self-contained. In the end though, the commands specified here are quite simple, so the choice will not have a massive impact. In the end, the uv team are considering their own command runner, so it could all end up under one tool anyways.

@jorenham
Copy link
Owner

Ok that makes sense 👌🏻. I'm not familiar with just, but judging from its popularity, it's pretty solid.

BTW, don't forget update the CONTRIBUTING.md, which explicitly mentions poe now.

scipy-stubs/py.typed Outdated Show resolved Hide resolved
@miloth
Copy link
Author

miloth commented Nov 16, 2024

The contribution docs should be sorted now. The things I think are worth considering in this PR are:

  • How the command runner and tox work together. The commands here are quite simple, so I think it could be feasible to get rid of them and put all the config in tox. I'll create a child PR into this one to showcase how. Leveraging the tox-uv plugin, it should work fairly nicely the CIs.
  • I'd like to investigate a bit better why the stubs are not recognized when installed with uv sync...
  • How do you feel about dividing the pyproject.toml in smaller self-contained sections? It's 300+ lines, so it could be a bit difficult finding specific pieces. I was thinking about taking out ruff.toml and tox.toml. I can do this in a separate PR, to have one per topic.

@jorenham jorenham linked an issue Nov 17, 2024 that may be closed by this pull request
@jorenham
Copy link
Owner

  • How do you feel about dividing the pyproject.toml in smaller self-contained sections? It's 300+ lines, so it could be a bit difficult finding specific pieces. I was thinking about taking out ruff.toml and tox.toml. I can do this in a separate PR, to have one per topic.

Hmm, I see what you mean, but I usually try to minimize the amount of (config) files in the repo root. Having a central place for configuration can also be helpful in my experience, which can be especially practical when we change a config option that affects multiple tools (e.g. mypy and pyright).

Perhaps it could help if we'd add a comment between sections, so that they're easier to visually distinguish 🤔

@jorenham
Copy link
Owner

Hmm, I see that just doesn't automatically run in the uv environment. So becauseuv run just stubtest kinda defeats the purpose; maybe we could prefix the just commands with a uv run in the justfile? That should also help simplify the ci.yml workflow.

Oh and speaking of GHA, the just docs mention the extractions/setup-just@v2 and taiki-e/install-action@just actions. So maybe one of those could help as well.


In the meantime I upgraded ruff and fixed the new errors, which is what's causing the merge conflicts, as this PR also included some fixes for those ruff errors. Anyway; there's no need to worry about those ruff errors now; and you can just reset those .pyi files to get rid of the conflicts.


  • How the command runner and tox work together. The commands here are quite simple, so I think it could be feasible to get rid of them and put all the config in tox. I'll create a child PR into this one to showcase how. Leveraging the tox-uv plugin, it should work fairly nicely the CIs.

That sounds interesting; looking forward to it :)


  • I'd like to investigate a bit better why the stubs are not recognized when installed with uv sync...

My gut feeling tells me it has something to do with hatch (and I based that on totally nothing 👌🏻). So perhaps switching to another build tool could help with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta: CI Continuous integration meta: deps Dependency management meta: packaging Building and release publishing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate from poetry to uv
2 participants