Added dependency resolving for %pip cells
#1662
Conversation
This PR adds a downloader and resolver for PyPI packages and wheels, that are installable via `pip` subprocess.
|
✅ 166/166 passed, 25 skipped, 2h7m11s total Running from acceptance #3121 |
| # TODO: https://github.com/databrickslabs/ucx/issues/1642 | ||
| return [] | ||
| # TODO: this is very basic code, we need to improve it |
There was a problem hiding this comment.
i.e. rewriting it to a regex? Or rewriting it ast parsing?
| return [] | ||
| if splits[1] != 'install': | ||
| return [] | ||
| # TODO: we need to support different formats of the library name and etc |
There was a problem hiding this comment.
i.e., lib definitions including versioning, e.g. pandas==0.18.0
| if not dist_info: | ||
| problem = DependencyProblem('library-install-failed', f'Failed to install {name}') | ||
| return MaybeDependency(None, [problem]) | ||
| container = SitePackageContainer(self._file_loader, dist_info) |
There was a problem hiding this comment.
Could you explain the intention of the container and wrapping loader classes?
| @@ -81,6 +129,7 @@ def __init__(self, packages: list[SitePackage]): | |||
| self._packages[top_level] = package | |||
|
|
|||
| def __getitem__(self, item: str) -> SitePackage | None: | |||
| item = item.replace("-", "_") | |||
There was a problem hiding this comment.
Why replace the dashes with underscores?
|
|
||
| def __init__( | ||
| self, | ||
| file_loader: FileLoader, |
There was a problem hiding this comment.
Why does the pip resolver need a file loader?
| return PipResolver(self._file_loader, resolver) | ||
|
|
||
| def resolve_library(self, path_lookup: PathLookup, name: str) -> MaybeDependency: | ||
| path_lookup.append_path(self._temporary_virtual_environment) |
There was a problem hiding this comment.
Should the resolve library append the path? Or should it be done outside, i.e. first resolve library then append the path if the library can actually be resolved
| problem = DependencyProblem('library-install-failed', f'Failed to install {name}') | ||
| return MaybeDependency(None, [problem]) | ||
| container = SitePackageContainer(self._file_loader, dist_info) | ||
| dependency = Dependency(WrappingLoader(container), Path(name)) |
There was a problem hiding this comment.
Should this be just the Path(name) or a absolute path
JCZuurmond
force-pushed
the
feat/pip-download
branch
from
103eb09 to
abf77da
Compare
| from tests.unit import locate_site_packages | ||
|
|
||
|
|
||
| def test_pip_resolver_resolve_library(mock_path_lookup): |
There was a problem hiding this comment.
Tests require internet access, maybe should be a integration test because of that
| assert maybe.dependency == dependency | ||
|
|
||
|
|
||
| @pytest.mark.fail("Fails because pytest has a try-except ImportError") |
There was a problem hiding this comment.
@ericvergnaud : This is a lovely exception, that we do not handle and which causes problems
__all__ = ["__version__", "version_tuple"]
try:
from ._version import version as __version__
from ._version import version_tuple
except ImportError: # pragma: no cover
# broken installation, we don't even try
# unknown only works because we do poor mans version compare
__version__ = "unknown"
version_tuple = (0, 0, "unknown")Partial copy from [PR](#1662)
Partial copy from [PR](#1662)
|
@JCZuurmond I guess this is already merged ? |
|
Yes, there are still some code snippets in there which was not merged yet (like appending a file to path only when it is not already there), but the PR is broken down into: |
This PR adds a downloader and resolver for PyPI packages and wheels, that are installable via
pipsubprocess.Closes #1642
Closes #1640