From dd811cf0ccb0d16416187e6f0e9d97626f35e4c7 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Thu, 27 Jul 2023 13:23:30 +0100 Subject: [PATCH] Restore resolution of Conda environments generated from non-lowercase package names Fix https://github.com/galaxyproject/galaxy/issues/16435 . --- lib/galaxy/tool_util/deps/conda_util.py | 32 ++++++- lib/galaxy/tool_util/deps/resolvers/conda.py | 18 ++-- test/unit/tool_util/test_conda_resolution.py | 95 +++++++++++++++++--- 3 files changed, 124 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/tool_util/deps/conda_util.py b/lib/galaxy/tool_util/deps/conda_util.py index b5c3101b57e..dca961338ab 100644 --- a/lib/galaxy/tool_util/deps/conda_util.py +++ b/lib/galaxy/tool_util/deps/conda_util.py @@ -388,6 +388,12 @@ class CondaContext(installable.InstallableContext): env_path = self.env_path(env_name) return os.path.isdir(env_path) + def get_conda_target_installed_path(self, conda_target: "CondaTarget") -> Optional[str]: + for env_name in (conda_target.install_environment, conda_target.capitalized_install_environment): + if self.has_env(env_name): + return self.env_path(env_name) + return None + @property def deactivate(self) -> str: return self._bin("deactivate") @@ -429,6 +435,7 @@ class CondaTarget: ) -> None: if SHELL_UNSAFE_PATTERN.search(package) is not None: raise ValueError(f"Invalid package [{package}] encountered.") + self.capitalized_package = package self.package = package.lower() if version and SHELL_UNSAFE_PATTERN.search(version) is not None: raise ValueError(f"Invalid version [{version}] encountered.") @@ -470,12 +477,25 @@ class CondaTarget: """The dependency resolution and installation frameworks will expect each target to be installed it its own environment with a fixed and predictable name given package and version. + Since Galaxy 23.1 the package name is lowercased as all Conda package + names must be lowercase. """ if self.version: return f"__{self.package}@{self.version}" else: return f"__{self.package}@_uv_" + @property + def capitalized_install_environment(self) -> str: + """Same as install_environment() but using the original capitalized + package name for backward compatibility with environments created before + Galaxy 23.1 . + """ + if self.version: + return f"__{self.capitalized_package}@{self.version}" + else: + return f"__{self.capitalized_package}@_uv_" + def __hash__(self) -> int: return hash((self.package, self.version, self.build, self.channel)) @@ -490,13 +510,19 @@ class CondaTarget: return False -def hash_conda_packages(conda_packages: Iterable[CondaTarget]) -> str: +def hash_conda_packages(conda_packages: Iterable[CondaTarget], capitalized_package_names: bool = False) -> str: """Produce a unique hash on supplied packages. TODO: Ideally we would do this in such a way that preserved environments. """ h = hashlib.new("sha256") for conda_package in conda_packages: - h.update(smart_str(conda_package.install_environment)) + h.update( + smart_str( + conda_package.capitalized_install_environment + if capitalized_package_names + else conda_package.install_environment + ) + ) return h.hexdigest() @@ -621,7 +647,7 @@ def is_search_hit_exact(conda_target: CondaTarget, search_hit: Dict[str, Any]) - def is_conda_target_installed(conda_target: CondaTarget, conda_context: CondaContext) -> bool: - return conda_context.has_env(conda_target.install_environment) + return conda_context.get_conda_target_installed_path(conda_target) is not None def filter_installed_targets(conda_targets: Iterable[CondaTarget], conda_context: CondaContext) -> List[CondaTarget]: diff --git a/lib/galaxy/tool_util/deps/resolvers/conda.py b/lib/galaxy/tool_util/deps/resolvers/conda.py index 866144ac4a6..859d937eec7 100644 --- a/lib/galaxy/tool_util/deps/resolvers/conda.py +++ b/lib/galaxy/tool_util/deps/resolvers/conda.py @@ -185,11 +185,10 @@ class CondaDependencyResolver( final_return_code = return_code return final_return_code - def install_all(self, conda_targets): + def install_all(self, conda_targets: List[CondaTarget], env: str) -> bool: if self.read_only: return False - env = self.merged_environment_name(conda_targets) return_code = install_conda_targets(conda_targets, conda_context=self.conda_context, env_name=env) if return_code != 0: is_installed = False @@ -237,9 +236,12 @@ class CondaDependencyResolver( preserve_python_environment = kwds.get("preserve_python_environment", False) - env = self.merged_environment_name(conda_targets) + for capitalized_package_names in (True, False): + env = self.merged_environment_name(conda_targets, capitalized_package_names) + is_installed = self.conda_context.has_env(env) + if is_installed: + break - is_installed = self.conda_context.has_env(env) install = kwds.get("install", None) if install is None: # Default behavior, install dependencies if conda_auto_install is active. @@ -248,7 +250,7 @@ class CondaDependencyResolver( # Install has been set to True, install if not yet installed. install = not is_installed if install: - is_installed = self.install_all(conda_targets) + is_installed = self.install_all(conda_targets, env) dependencies: List[Dependency] = [] if is_installed: @@ -266,10 +268,10 @@ class CondaDependencyResolver( return dependencies - def merged_environment_name(self, conda_targets): + def merged_environment_name(self, conda_targets: List[CondaTarget], capitalized_package_names: bool = False) -> str: if len(conda_targets) > 1: # For continuity with mulled containers this is kind of nice. - return f"mulled-v1-{hash_conda_packages(conda_targets)}" + return f"mulled-v1-{hash_conda_packages(conda_targets, capitalized_package_names)}" else: assert len(conda_targets) == 1 return conda_targets[0].install_environment @@ -317,7 +319,7 @@ class CondaDependencyResolver( if job_directory: conda_environment = os.path.join(job_directory, conda_env) else: - conda_environment = self.conda_context.env_path(conda_target.install_environment) + conda_environment = self.conda_context.get_conda_target_installed_path(conda_target) return CondaDependency( self.conda_context, diff --git a/test/unit/tool_util/test_conda_resolution.py b/test/unit/tool_util/test_conda_resolution.py index ecb5f366a4c..5e4084e70da 100644 --- a/test/unit/tool_util/test_conda_resolution.py +++ b/test/unit/tool_util/test_conda_resolution.py @@ -1,6 +1,8 @@ import os.path import tempfile +import pytest + from galaxy.tool_util.deps import DependencyManager from galaxy.tool_util.deps.conda_util import ( best_search_result, @@ -9,10 +11,16 @@ from galaxy.tool_util.deps.conda_util import ( install_conda, installed_conda_targets, ) -from galaxy.tool_util.deps.requirements import ToolRequirement +from galaxy.tool_util.deps.requirements import ( + ToolRequirement, + ToolRequirements, +) +from galaxy.tool_util.deps.resolvers import NullDependency from galaxy.tool_util.deps.resolvers.conda import ( + CondaDependency, CondaDependencyResolver, DEFAULT_ENSURE_CHANNELS, + MergedCondaDependency, ) from .util import external_dependency_management @@ -27,27 +35,94 @@ def test_install_conda_build(tmp_path) -> None: assert cc.conda_build_available -@external_dependency_management -def test_conda_resolution(tmp_path) -> None: - job_dir = os.path.join(tmp_path, "000") - dependency_manager = DependencyManager(tmp_path) - resolver = CondaDependencyResolver( +@pytest.fixture(scope="module") +def resolver(tmp_path_factory): + dependency_manager = DependencyManager(tmp_path_factory.mktemp("dependencies")) + return CondaDependencyResolver( dependency_manager, auto_init=True, auto_install=True, use_path_exec=False, # For the test ensure this is always a clean install ) + + +@external_dependency_management +def test_single_package_resolution(resolver: CondaDependencyResolver) -> None: + # Test that a read-only resolver returns a NullDependency resolver.read_only = True - req = ToolRequirement(name="samtools", version=None, type="package") - dependency = resolver.resolve(req, job_directory=job_dir) + package_name = "eXpress" + req = ToolRequirement(name=package_name, version=None, type="package") + dependency = resolver.resolve(req) + assert isinstance(dependency, NullDependency) assert dependency.shell_commands() is None + # Test that a (fake) environment generated from a non-lowercase package name + # is still accepted by the resolver + env_path = resolver.conda_context.env_path(f"__{package_name}@_uv_") + os.mkdir(env_path) + dependency = resolver.resolve(req) + assert isinstance(dependency, CondaDependency) + assert dependency.name == package_name + assert dependency.version is None + assert dependency.exact + assert dependency.environment_path == env_path + os.rmdir(env_path) + + # Test that a non-read-only resolver creates a conda environment (with a + # lowercase package name) and returns a CondaDependency resolver.read_only = False - req = ToolRequirement(name="samtools", version=None, type="package") - dependency = resolver.resolve(req, job_directory=job_dir) + dependency = resolver.resolve(req) + assert isinstance(dependency, CondaDependency) + assert dependency.name == package_name + assert dependency.version is None + assert dependency.exact + assert dependency.environment_path == resolver.conda_context.env_path(f"__{package_name.lower()}@_uv_") assert dependency.shell_commands() is not None +@external_dependency_management +def test_multi_package_resolution(resolver: CondaDependencyResolver) -> None: + # Test that a read-only resolver returns an empty list + resolver.read_only = True + reqs = ToolRequirements( + [ + ToolRequirement(name="samtools", version=None, type="package"), + ToolRequirement(name="eXpress", version=None, type="package"), + ] + ) + dependency_list = resolver.resolve_all(reqs) + assert dependency_list == [] + + # Test that a (fake) mulled environment generated from non-lowercase package + # names is still accepted by the resolver + env_path = resolver.conda_context.env_path( + "mulled-v1-37955d8b3667efa38f1fe42bc37b4c9a03e67a90f0055d1db9d388b2f98fe3a1" + ) + os.mkdir(env_path) + dependency_list = resolver.resolve_all(reqs) + assert len(dependency_list) == 2 + for dependency in dependency_list: + assert isinstance(dependency, MergedCondaDependency) + assert dependency.version is None + assert dependency.exact + assert dependency.environment_path == env_path + os.rmdir(env_path) + + # Test that a non-read-only resolver creates a mulled conda environment + # (with a hash from lowercase package names) and returns a list of + # MergedCondaDependency + resolver.read_only = False + dependency_list = resolver.resolve_all(reqs) + assert len(dependency_list) == 2 + for dependency in dependency_list: + assert isinstance(dependency, MergedCondaDependency) + assert dependency.version is None + assert dependency.exact + assert dependency.environment_path == resolver.conda_context.env_path( + "mulled-v1-f36491d362c22cdb24f7de3e1ff978a5ee02cadc8c12a12ae06bce57bbf0765c" + ) + + @external_dependency_management def test_against_conda_prefix_regression() -> None: """Test that would fail if https://github.com/rtfd/readthedocs.org/issues/1902 regressed."""