[MNT] Declare extra conflicts to make uv sync work#8892
[MNT] Declare extra conflicts to make uv sync work#8892yarnabrina wants to merge 7 commits intomainfrom
Conversation
e151619 to
2c63169
Compare
fkiraly
left a comment
There was a problem hiding this comment.
please do not remove the dependencies_lowest etc depsets, they are used in testing to ensure compatibility with 2023 state etc
|
@fkiraly I don't plan to remove, but those are the ones causing problems with uv. I asked @marrov and @jgyasu on Discord, and also on uv issues, waiting for replies. Do you have any problem if we rename these? I think And while we are ln the topic, do we really need to keep supporting pandas1, all_extras, etc.? I know I asked this repetitively, but we need to decide when to stop supporting such old versions. |
I have no problem with renaming these, but this would seem very odd. Is this a bug in |
I can not say. I've created astral-sh/uv#16118 to get some clarity. Even currently when I commented those two extras, there are conflicting extras/groups but it's not failing. That's what make me think there may be some regex pattern or something, or some configuration I am missing. Not familiar enough with |
2c63169 to
d4eeb8c
Compare
|
FYI @sktime/core-developers Conflicts detected by
|
|
FYI @Abelarm for your related PR. |
There was a problem hiding this comment.
I think this is still problematic in its current state given the above.
Does uv not allow mutually contradictory dependency sets? This makes sense for testing on pins as we currently do, but it seems like it would be prevented?
Also, we should not merge until we have answered the following questions:
- what, if any, is the impact of moving the build backend to our users?
- does this cause a breaking change or not, in package setup or package usage?
It allows, and the current configuration explicitly notes what all are contradictory. See
For
No effect for
@fkiraly can you please elaborate? What changes seem problematic to you? If you are talking about the CI failures, the errors seem very constrained to classification/regression/networks and failure make it very clear that this is source code and/or unit test issue, not related to package build backend. |
Lack of clarity on the impact is my greatest concern.
What is the impact on contributors? Would
How do you know? |
Regarding clarity: for instance, you claimed above we have to remove the dependency sets starting with That is one indication that tells me we are not entirely certain about the impact of this change, simply since you - who are proposing the change - are also not with certainty familiar with Whereas the change is happening at such a central place in the delivery and release pipeline that even a small unmanaged problem could become catastrophic and go unnoticed for a while. So we need to be absolutely sure we understand what we are doing here, and the back/forth about the conflicting dependencies is not filling me with confidence. |
I do not claim to be 100% expert on I've not made the PR ready for review without satisfying myself. I've also requested
I'll request other @sktime/core-developers and @sktime/community-council members (and all contributors/users) to also share their opinion/concern on user impact by build backend change. It's completely fine of course if we decide to say let's not change, but that is a decision that should be taken based on testing and specific scenarios and #6288 then needs to be closed as not planned or etc. accordingly. |
|
Hi @yarnabrina and @fkiraly , In the optics of moving to lockfiles, I think this PR is necessary; Even more is the cornerstone to the whole "move towards a more robust dependencies resolution". In order to move it to fully For my side, here is the (partial)list of task we need to compete before moving to "lockfile-based approach" (
I am really happy to help with whatever, and let me know if I missed something. |
|
hm, I would not want to require a change in developer commands from Have we discussed strategy on |
I completely agree and I don't want to get rid of In a way or another if we move to
In my opinion we(you) need to decide what you want to achieve:
Maybe as you are suggesting we should open an Issue, or a discussion on any other platform for deciding the approach. |
|
My understanding was that the changes I've made in this PR changes the build backend from If you agree with me, then the only people that gets affected are contributors or someone using |
|
I am also somewhat confused. I think we need to write down exactly the workflows (user or developer) that we are considering, the changes we are proposing, and how the workflows will be affected (or not). |
d4eeb8c to
b4f05d9
Compare
Requesting review, and comments on what other common workflow you are thinking of. # Expected `pip` workflows and effect of switching from `setuptools` to `uv-build` as build backend
## By `sktime` users
1. install from PyPI
* Old: `pip install sktime`
* New: `pip install sktime`
2. same but with additional optional dependencies
* Old: `pip install sktime[forecasting]`
* New: `pip install sktime[forecasting]`
In both cases, users will get the same pre-built wheels from PyPI as before. The `uv` pip interface is optional; if desired, users may use `uv add sktime` (see the `uv` docs).
## By `sktime` contributors
1. install from local source code
* Old: `pip install .` from the root of the repository
* New: `pip install .` from the root of the repository
2. same but in editable mode
* Old: `pip install -e .` from the root of the repository
* New: `pip install -e .` from the root of the repository
In both cases, contributors will get the same behaviour as before. Contributors may optionally use the `uv` CLI for similar workflows.
If contributors modify `pyproject.toml` to add or remove optional dependencies, those changes will be respected by both build backends.
Tracking a `uv.lock` file is optional. If we do track it, the lock records exact dependency versions and CI can use `uv sync` to reproduce builds. Contributors who change `pyproject.toml` must regenerate the lock with `uv lock` and commit the update. Contributors who do not use `uv` locally will not be able to regenerate the lock easily; this is an optional reproducibility feature and is not required to use `uv-build`. `pip` (or `uv pip`) will continue to install the latest compatible versions unless a lockfile is used.
## By `sktime` maintainers
1. build from source code
* Old: `python -m build`
* New: `uv build`
2. upload to PyPI
* Old: `twine upload dist/*`
* New: `twine upload dist/*`
In both cases, maintainers will get the same behaviour as before. Note that `uv build` and `uv publish` are also available as optional alternatives. |
|
@fkiraly any idea why is CI not running even after 10 hours? On sunday test all runs, but it didn't usually take this much time, especially after the optimisations (and just checked, even that's not running and waiting for something else). |
You are right let's not mix topics, this PR is just for moving the
Where you able to test both "How will |
Yes. At least on my system. But it must be verified on at least one other system.
My understanding that it'll be simply ignored by pip and it's not an issue. The conflict declaration is just for valodation. For users, build backend uv will take care of this and user deals with whl file only. For contributors, editable installs won't matter on exclusion logic. For maintainers, pip doesn't relate to build/publish and I checked @sktime/core-developers please do share your thoughts/notes on the above comparison, reproduction, testing , etc. |
|
Bumping this @sktime/core-developers . Any specific comments/concerns? |
b4f05d9 to
4a5ab78
Compare
fkiraly
left a comment
There was a problem hiding this comment.
Given discussion on discord, @julian-fong reported that uv sync takes 90 minutes and results in an unusuable set of versions.
It seems the correct solutions would be to maintain a uv.lock file instead?
|
A bit of an aside from the core discussion here, but this pull request
You might want to update the title / pull request body to clarify given the changes? |
Hi @zanieb the mismatch between title and implementation is real. My goal was to fully migrate, including build system to uv build but there's multiple long discussions on Discord at the end of which @fkiraly suggested to only make pyproject.toml uv compatible (so that is any contributor want to use uv sync they don't get blocked) and not make the migration. This was reverted yesterday: 527c069. Anyway, I'll mark this PR as draft for now till the new PR is finalised. |
Related #6288 , #9383