Skip to content
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

Persist packages from original lockfile _only_ for platforms not requested for lock #485

Merged
merged 8 commits into from
Sep 11, 2023
56 changes: 38 additions & 18 deletions conda_lock/conda_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def fn_to_dist_name(fn: str) -> str:
return fn


def make_lock_files(
def make_lock_files( # noqa: C901
*,
conda: PathLike,
src_files: List[pathlib.Path],
Expand Down Expand Up @@ -341,35 +341,35 @@ def make_lock_files(
virtual_package_repo=virtual_package_repo,
required_categories=required_categories if filter_categories else None,
)
lock_content: Optional[Lockfile] = None
original_lock_content: Optional[Lockfile] = None

platforms_to_lock: List[str] = []
platforms_already_locked: List[str] = []
if lockfile_path.exists():
import yaml

try:
lock_content = parse_conda_lock_file(lockfile_path)
original_lock_content = parse_conda_lock_file(lockfile_path)
except (yaml.error.YAMLError, FileNotFoundError):
logger.warning(
"Failed to parse existing lock. Regenerating from scratch"
)
lock_content = None
original_lock_content = None
else:
lock_content = None
original_lock_content = None

if lock_content is not None:
platforms_already_locked = list(lock_content.metadata.platforms)
platforms_to_lock: List[str] = []
platforms_already_locked: List[str] = []
if original_lock_content is not None:
platforms_already_locked = list(original_lock_content.metadata.platforms)
update_spec = UpdateSpecification(
locked=lock_content.package, update=update
locked=original_lock_content.package, update=update
)
for platform in lock_spec.platforms:
if (
update
or platform not in lock_content.metadata.platforms
or platform not in platforms_already_locked
or not check_input_hash
or lock_spec.content_hash_for_platform(platform)
!= lock_content.metadata.content_hash[platform]
!= original_lock_content.metadata.content_hash[platform]
):
platforms_to_lock.append(platform)
if platform in platforms_already_locked:
Expand All @@ -385,9 +385,12 @@ def make_lock_files(
)
platforms_to_lock = sorted(set(platforms_to_lock))

if platforms_to_lock:
if not platforms_to_lock:
new_lock_content = original_lock_content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since previously we were mutating lock_content in this conditional block, the "else" condition was lack of mutation. I think making that case explicit and first made this renaming change the most readable.

else:
print(f"Locking dependencies for {platforms_to_lock}...", file=sys.stderr)
lock_content = lock_content | create_lockfile_from_spec(

fresh_lock_content = create_lockfile_from_spec(
conda=conda,
spec=lock_spec,
platforms=platforms_to_lock,
Expand All @@ -398,9 +401,24 @@ def make_lock_files(
strip_auth=strip_auth,
)

if not original_lock_content:
new_lock_content = fresh_lock_content
else:
# Persist packages from original lockfile for platforms not requested for lock
packages_not_to_lock = [
dep
for dep in original_lock_content.package
if dep.platform not in platforms_to_lock
]
lock_content_to_persist = original_lock_content.copy(
deep=True,
update={"package": packages_not_to_lock},
)
new_lock_content = lock_content_to_persist | fresh_lock_content

if "lock" in kinds:
write_conda_lock_file(
lock_content,
new_lock_content,
lockfile_path,
metadata_choices=metadata_choices,
)
Expand All @@ -410,7 +428,9 @@ def make_lock_files(
file=sys.stderr,
)

assert lock_content is not None
# After this point, we're working with `new_lock_content`, never
# `original_lock_content` or `fresh_lock_content`.
assert new_lock_content is not None
Copy link
Contributor

@mfisher87 mfisher87 Sep 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this assertion a bit confusing. I was not able to type new_lock_content: Lockfile because original_lock_content could be None when we do this assignment:

        if not platforms_to_lock:
            new_lock_content = original_lock_content

I would like to move this check in to the type system, but that seems better left for a larger refactor effort.


# check for implicit inclusion of cudatoolkit
# warn if it was pulled in, but not requested explicitly
Expand All @@ -422,13 +442,13 @@ def make_lock_files(
for pkg in itertools.chain(*lock_spec.dependencies.values())
)
if not cudatoolkit_requested:
for package in lock_content.package:
for package in new_lock_content.package:
if package.name == "cudatoolkit":
logger.warning(_implicit_cuda_message)
break

do_render(
lock_content,
new_lock_content,
kinds=[k for k in kinds if k != "lock"],
include_dev_dependencies=include_dev_dependencies,
filename_template=filename_template,
Expand Down
15 changes: 15 additions & 0 deletions environments/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# How to install dev environment

* First install dev dependencies:

```
mamba env create -f environments/dev-environment.yaml
mamba activate conda-lock-dev
```

* Then, install `conda-lock` in editable mode. This will also install its runtime
dependencies as defined in `pyproject.toml`.

```
pip install --editable .
```
6 changes: 6 additions & 0 deletions tests/test-dependency-removal/environment-postupdate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
channels:
- conda-forge
platforms:
- linux-64
dependencies:
- zlib
7 changes: 7 additions & 0 deletions tests/test-dependency-removal/environment-preupdate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
channels:
- conda-forge
platforms:
- linux-64
dependencies:
- zlib
- xz
6 changes: 6 additions & 0 deletions tests/test-move-pip-dependency/environment-postupdate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
channels:
- conda-forge
platforms:
- linux-64
dependencies:
- six
7 changes: 7 additions & 0 deletions tests/test-move-pip-dependency/environment-preupdate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
channels:
- conda-forge
platforms:
- linux-64
dependencies:
- pip:
- six
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
channels:
- conda-forge
platforms:
- linux-64
- osx-64
dependencies:
- zlib ==1.2.13
7 changes: 7 additions & 0 deletions tests/test-update-filter-platform/environment-preupdate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
channels:
- conda-forge
platforms:
- linux-64
- osx-64
dependencies:
- zlib ==1.2.8
100 changes: 99 additions & 1 deletion tests/test_conda_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

from glob import glob
from pathlib import Path
from typing import Any, ContextManager, Dict, List, Union
from typing import Any, ContextManager, Dict, List, Tuple, Union
from unittest.mock import MagicMock
from urllib.parse import urldefrag, urlsplit

Expand Down Expand Up @@ -1088,6 +1088,36 @@ def update_environment(tmp_path: Path) -> Path:
)


@pytest.fixture
def update_environment_filter_platform(tmp_path: Path) -> Tuple[Path, Path]:
test_dir = clone_test_dir("test-update-filter-platform", tmp_path)

return (
test_dir / "environment-preupdate.yml",
test_dir / "environment-postupdate.yml",
)


@pytest.fixture
def update_environment_dependency_removal(tmp_path: Path) -> Tuple[Path, Path]:
test_dir = clone_test_dir("test-dependency-removal", tmp_path)

return (
test_dir / "environment-preupdate.yml",
test_dir / "environment-postupdate.yml",
)


@pytest.fixture
def update_environment_move_pip_dependency(tmp_path: Path) -> Tuple[Path, Path]:
test_dir = clone_test_dir("test-move-pip-dependency", tmp_path)

return (
test_dir / "environment-preupdate.yml",
test_dir / "environment-postupdate.yml",
)


@flaky
@pytest.mark.timeout(120)
def test_run_lock_with_update(
Expand Down Expand Up @@ -1132,6 +1162,74 @@ def test_run_lock_with_update(
assert post_lock["flask"].version == pre_lock["flask"].version


@flaky
@pytest.mark.timeout(120)
def test_run_lock_with_update_filter_platform(
monkeypatch: "pytest.MonkeyPatch",
update_environment_filter_platform: Tuple[Path, Path],
conda_exe: str,
):
"""Test that when updating for one platform, other platforms are not updated."""
pre_env = update_environment_filter_platform[0]
post_env = update_environment_filter_platform[1]
environment_dir = pre_env.parent
monkeypatch.chdir(environment_dir)

run_lock([pre_env], conda_exe=conda_exe)
run_lock([post_env], conda_exe=conda_exe, update=["zlib"], platforms=["linux-64"])

post_lock = {
(p.name, p.platform): p
for p in parse_conda_lock_file(environment_dir / DEFAULT_LOCKFILE_NAME).package
}
assert post_lock[("zlib", "linux-64")].version == "1.2.13"
assert post_lock[("zlib", "osx-64")].version == "1.2.8"


@flaky
@pytest.mark.timeout(120)
def test_remove_dependency(
monkeypatch: "pytest.MonkeyPatch",
update_environment_dependency_removal: Tuple[Path, Path],
conda_exe: str,
):
pre_env = update_environment_dependency_removal[0]
post_env = update_environment_dependency_removal[1]
environment_dir = pre_env.parent
monkeypatch.chdir(environment_dir)

run_lock([pre_env], conda_exe=conda_exe)
run_lock([post_env], conda_exe=conda_exe)
post_lock = [
p.name
for p in parse_conda_lock_file(environment_dir / DEFAULT_LOCKFILE_NAME).package
]

assert "xz" not in post_lock


@flaky
@pytest.mark.timeout(120)
def test_move_dependency_from_pip_section(
monkeypatch: "pytest.MonkeyPatch",
update_environment_move_pip_dependency: Tuple[Path, Path],
conda_exe: str,
):
pre_env = update_environment_move_pip_dependency[0]
post_env = update_environment_move_pip_dependency[1]
environment_dir = pre_env.parent
monkeypatch.chdir(environment_dir)

run_lock([pre_env], conda_exe=conda_exe)
run_lock([post_env], conda_exe=conda_exe)
post_lock = [
p.name
for p in parse_conda_lock_file(environment_dir / DEFAULT_LOCKFILE_NAME).package
]

assert post_lock.count("six") == 1


def test_run_lock_with_locked_environment_files(
monkeypatch: "pytest.MonkeyPatch", update_environment: Path, conda_exe: str
):
Expand Down