-
Notifications
You must be signed in to change notification settings - Fork 9
add uv
backend
#172
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
base: main
Are you sure you want to change the base?
add uv
backend
#172
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
==========================================
+ Coverage 94.16% 94.28% +0.12%
==========================================
Files 14 14
Lines 2159 2222 +63
==========================================
+ Hits 2033 2095 +62
- Misses 126 127 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Uhhhh I didn't see this, awesome! |
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.
One thing I'm not clear on: uv
is now a dependency, in what case would it be not available?
Almost never, but if the user manages to break their environment by removing |
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.
@jaimergp Could you make a separate PR with rename enum from PIP
to PIPI
?
We may merge it fast and make this diff much cleaner.
def constraints() -> Sequence[str]: | ||
""" | ||
Version constraints to limit unwanted changes in installation. | ||
""" | ||
return [f'napari=={_napari_version}'] |
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 uv pip install --upgrade
upgrade not only itself but also all its dependencies. But do not think about other packages installed in the environment.
So if a package depends on NumPy and Numba is installed, having only pinned napari means that the upgrade may end with a non-working environment, as import of Numba crashes.
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.
Hm, do you think we could use --upgrade-package
instead or a different --resolution
strategy?
$ uv pip install --help
[...]
-U, --upgrade Allow package upgrades, ignoring pinned versions in any existing output file. Implies
`--refresh`
-P, --upgrade-package <UPGRADE_PACKAGE> Allow upgrades for a specific package, ignoring pinned versions in any existing output
file. Implies `--refresh-package`
--resolution <RESOLUTION> The strategy to use when selecting between the different compatible versions for a given
package requirement [env: UV_RESOLUTION=] [possible values: highest, lowest,
lowest-direct]
[...]
And for clarity, does this affect pip install --upgrade
too (without uv
)?
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.
Or is the suggestion to extend the constraints()
list with all the dependencies of the currently installed napari
version, and possibly all the plugins? Something like:
constraints = [f"napari=={napari.__version__}, *napari_plugins(exact_version=True)]
for constraint in constraints:
constraints.extend(get_dependencies(name_of(constraint), with_version_ranges=True)
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.
As I remember, this is what Charlie advertised to me to use: --upgrade-package
.
And for clarity, does this affect
pip install --upgrade
too (withoutuv
)?
As far as I know, yes. Pip, do not update all dependencies of updated packages if the lower bound is below the currently installed version. (same for --upgrade-package
).
Or is the suggestion to extend the
constraints()
list with all the dependencies of the currently installednapari
version, and possibly all the plugins? Something like:
No. As it may block many updates when a plugin requires a newer version of its 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.
As I remember, this is what Charlie advertised to me to use: --upgrade-package.
Ok, I'll change to using that flag!
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.
As far as I know, yes. Pip, do not update all dependencies of updated packages if the lower bound is below the currently installed version. (same for
--upgrade-package
).
This is what I can find in the help:
-U, --upgrade Upgrade all specified packages to the newest available version. The handling
of dependencies depends on the upgrade-strategy used.
--upgrade-strategy <upgrade_strategy>
Determines how dependency upgrading should be handled [default: only-if-
needed]. "eager" - dependencies are upgraded regardless of whether the
currently installed version satisfies the requirements of the upgraded
package(s). "only-if-needed" - are upgraded only when they do not satisfy
the requirements of the upgraded package(s).
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.
@Czaki any word here?
So if someone had a system |
In a shell with the napari env active, or in the napari console, yes.
I need to look into how it is packaged, but my intuition tells me that we would only be able to do that by not depending on it in |
Co-authored-by: Grzegorz Bokota <[email protected]>
See #176. Once that's in I'll rebase here. |
Hi there, so gave this a check on Windows and, as long as you install |
Yes, it's added as a dependency 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.
Thanks @jaimergp. I think we should add pixi as well since it creates a nicer experience when using GPUs. It doesn't need to be this PR. 😄
If we add |
I'd like to keep it for a while in case it helps with debugging, workarounds, and other annoying tasks in case |
I check some uv documentation. I'm not a fan of adding it as a default dependency, as it will shadow the global one and need separate updates. |
I'd like to do that too, but the inability to specify an arbitrary environment location will make integration with the installation root problematic. I can think of hacky workarounds (e.g. having an empty project folder somewhere where we maintain the necessary lockfiles and then using |
We can go back to the behaviour of only using the |
Added it as an extra so folks can install it with |
You need to add it also to testing dependencies. |
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.
Looks really good. A few small questions.
|
||
deps = | ||
napari_repo: git+https://github.com/napari/napari.git | ||
extras = testing |
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.
As there is no skip for uv tests I prefer to add uv to testing extras.
Using napari-plugin-manager[uv]
in testing extras.
""" | ||
Version constraints to limit unwanted changes in installation. | ||
""" | ||
return [f'napari=={_napari_version}'] |
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.
Hm. So the only risk for now is if the environment has Numba installed and the plugin needs a NumPy newer than the installed one. But it is a problem as well for pip.
if tool == NapariPipInstallerTool | ||
else '_python_executable', | ||
lambda *a: 'not-a-real-executable', |
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 we can not unify this?
if tool == NapariPipInstallerTool: | ||
monkeypatch.setattr( | ||
NapariPipInstallerTool, | ||
'origins', | ||
('https://pypi.org/simple',), | ||
) |
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 if
is needed? Why we cannot uify this?
This should result in (way) faster plugin installs.