Allow scripted modules to declare requirements and constraints, and enforce Slicer Core constraints. #8181
Conversation
|
After writing the note about "halted import" for f10e7d7 I need to verify: If module A uses a guarded import on eg. For more detail: Suppose module A guards If the user causes module A to resolve the dependency first, things are fine. But if the user causes module B to resolve the dependency first, the
If that logic can't be done or is too cumbersome, I think it is better to remove the import halting feature altogether. It is intended to help detect bugs that might go unnoticed. It is not worth it if the presence of a new module breaks older modules' existing code. |
Unconditionally bundle UV in slicer if Python is enabled.
This is a combination of several commits: WIP: Initial lazy import implementation WIP: Test scaffolding for lazy import implementation WIP: Start building tests lazy loading wip WIP: pip_install uses uv and slicer core constraints WIP: pip_install respects guarded requirements WIP: Set relevant UV environment variables WIP: style and comments WIP: Fix tests after restructure Revert "WIP: Fix tests after restructure" This reverts commit 5253f72.
- Generate sensible error messages and prompts by inspecting a `--dry-run` - Allow the user to cancel installation. - Remove halted imports as they may break other modules. - Support separate requirements.txt and constraints.txt for tricky ML use cases. This is a combination of multiple commits: ENH: Dry run, user cancellation, sensible error messages and logs. WIP: Rename tests. WIP: Support separate requirements.txt and constraints.txt ENH: Naming improvements. FIX: Remove halted imports. Improve proxy module.
DOC: Add new python_packages doc to developer guide. DOC: Update pip_install to point to new python_packages doc. DOC: Notes about pip_install, pip_uninstall, uv, and errors. DOC: Notes about requirements, constraints, import groups, and halted import. DOC: Fix table of contents, typos DOC: Update docs with recent changes. Document manual usages. DOC: Improve introduction.
The names of various classes and functions are more clear now. - FileIdentifier - Requirements
|
cc @jcfr, @lassoan, @pieper. Readthedocs for this PR are now comprehensive. https://slicer--8181.org.readthedocs.build/en/8181/developer_guide/python_packages.html I'd appreciate any feedback or unsupported edge-cases you foresee on this. The goal is to primarily support things in discrete layers of abstraction:
pip_install('itk')
pip_install('itk', interactive=False)
pip_install(requirements=FileIdentifier('Sample Module', '.:requirements.txt'))
register_constraints(FileIdentifier('Sample Module', '.:constraints.txt'))
import itk
requirements = Requirements('.:requirements.txt', name="Sample Module")
try:
requirements.resolve()
except InstallAbortedError:
...
import itk
from __future__ import annotations
with Requirements('.:requirements.txt') as requirements:
import itk
try:
requirements.resolve()
except InstallAbortedError:
...
im: itk.Image = itk.imread(...)
from __future__ import annotations
with Requirements('.:requirements.txt'):
import itk
im: itk.Image = itk.imread(...)Note that all four of these layers will respect the constraints registered by core slicer and other modules; so it is already not possible to violate @jcfr RE my messages yesterday: I did remove import halting mentioned in #8181 (comment). It causes more problems than it solves. Now that I think the interface is hardened, I plan to translate my ad-hoc testing modules into comprehensive unit tests. I also should take a look at the API documentation to be sure it's not out of date. I need some help on automatically generating I think it is possible to do this from CMake, but my attempts to do this did not work. Each external python package needs to append Also, because of the requirement |
`uv pip` supports extras in constraints, but `pip` does not. Make `core-constraints.txt` pip-compatible via `uv pip compile`. `PyJWT[crypto]==2.8.0` is currently the only problematic constraint.
| dependency.function() | ||
| # ^ Resolution occurs here. | ||
|
|
||
| See `Automatic Installation <https://slicer.readthedocs.io/en/latest/developer_guide/python_packaging.html#manual-installation>`_. |
There was a problem hiding this comment.
This obviously doesn't work right in the CI readthedocs. I assume there is some way to get reST to do this properly?
| # An omitted package_name is a relative import for convenience. | ||
| if not package_name: | ||
| package_name = "." |
There was a problem hiding this comment.
Remove this. Do not do relative import. If the package is not provided, do a path lookup on sys.path. Perhaps there is something to help with this in importlib.util? But it is not handled by importlib.resources.
This would be to support requirements files at the top level of an extension, eg 'sample-requirements.txt'.
| if not package_name: | ||
| package_name = "." |
There was a problem hiding this comment.
| if not package_name: | |
| package_name = "." | |
| if not package_name: # No package specified, look in site-packages and Python path | |
| search_paths = site.getsitepackages() + [site.getusersitepackages()] + sys.path | |
| for search_dir in search_paths: | |
| candidate_path = Path(search_dir) / path | |
| if candidate_path.exists(): | |
| return contextlib.nullcontext(candidate_path) | |
| raise FileNotFoundError(f"File '{path}' not found in site-packages or Python path.") |
Work toward #7707. A summary:
LazyImportGroupfrom the prior revision is renamed toslicer.deps.GuardedImports. I still don't love this name.GuardedImportscontext manager registers itsrequirements.txtas a constraint that applies to all invocations ofpip_install.GuardedImportsalso usepip_installto install missing dependencies.scipy,numpy, etc.) also have a constraint which is enforced by all invocations ofpip_installandGuardedImports.Together, the intent is for extension developers to declare their dependencies in a
requirements.txt, have clear error messages and logs if any of those dependencies are incompatible with bundled packages or other extensions, and support static analysis via trueimportsyntax. See #7707 for more details.This PR is a draft for four reasons:
First, I have manually copied all the builtin requirements into
slicer.deps:core-constraints.txt. This file must be generated automatically before the PR should be accepted. I see a couple options to do this, either generate the file viaUtilities/Scripts/python_package_updater.py; or generate that file in CMake. For example see the${requirements_file}fornumpyin https://github.com/Slicer/Slicer/blob/main/SuperBuild/External_python-numpy.cmake#L30-L31. It is unclear how/when this file should be installed - probably not when the superbuild is configured, since we probably don't want disabled packages to be constrained.Second, I need to improve reporting to the user. There are some# todo throughout the
pip_installandpip_uninstallimplementations. These are points where we would inspect theuv pip installreturn code and stdout to assemble and appropriate message for the user. Critically, it should be possible for the user to cancel the installation.Third, I want to evaluate just how necessary the dependency on
uvis. I suspect that it is worth it, but I want to benchmark dependency resolution on a large system, for example resolving different-but-compatible versions oftorchand its dependencies. I haven't investigated thetorchdependency graph enough yet to build this benchmark.Fourth, developer documentation must be updated to explain where to place the requirements.txt and how to deal with the new error messages that will appear. To be clear, these are not new modes of failure, but they might not be ones that extension developers have yet considered. The error messages and documentation should be clear enough that a developer can quickly deal with the issue. (The brief advice should be: almost always use
~=version specifiers.)I welcome comments on the general approach or advice on how to generate that
core-constraints.txtfile.