🎨Ensure pulling s4l returns smooth increment of progress (Part 1) (#5664

)

packages/notifications-library/tests/conftest.py

@@ -14,7 +14,6 @@
 from notifications_library.payments import PaymentData
 from pydantic import EmailStr, parse_obj_as
 from pytest_simcore.helpers.typing_env import EnvVarsDict
-from pytest_simcore.helpers.utils_envs import load_dotenv
 from simcore_postgres_database.models.products import Vendor
 
 pytest_plugins = [
@@ -39,15 +38,7 @@ def package_dir() -> Path:
 
 def pytest_addoption(parser: pytest.Parser):
     group = parser.getgroup(
-        "external_environment",
-        description="Replaces mocked services with real ones by passing actual environs and connecting directly to external services",
-    )
-    group.addoption(
-        "--external-envfile",
-        action="store",
-        type=Path,
-        default=None,
-        help="Path to an env file. Consider passing a link to repo configs, i.e. `ln -s /path/to/osparc-ops-config/repo.config`",
+        "simcore",
     )
     group.addoption(
         "--external-user-email",
@@ -66,26 +57,11 @@ def pytest_addoption(parser: pytest.Parser):
 
 
 @pytest.fixture(scope="session")
-def external_environment(request: pytest.FixtureRequest) -> EnvVarsDict:
-    """
-    If a file under test folder prefixed with `.env-secret` is present,
-    then this fixture captures it.
-
-    This technique allows reusing the same tests to check against
-    external development/production servers
-    """
-    envs = {}
-    if envfile := request.config.getoption("--external-envfile"):
-        print("🚨 EXTERNAL `envfile` option detected. Loading", envfile, "...")
-
-        assert isinstance(envfile, Path)
-        assert envfile.is_file()
-
-        envs = load_dotenv(envfile)
-        assert "PAYMENTS_GATEWAY_API_SECRET" in envs
-        assert "PAYMENTS_GATEWAY_URL" in envs
-
-    return envs
+def external_environment(external_environment: EnvVarsDict) -> EnvVarsDict:
+    if external_environment:
+        assert "PAYMENTS_GATEWAY_API_SECRET" in external_environment
+        assert "PAYMENTS_GATEWAY_URL" in external_environment
+    return external_environment
 
 
 @pytest.fixture(scope="session")

packages/pytest-simcore/src/pytest_simcore/__init__.py

@@ -1,20 +1,31 @@
 # Collection of tests fixtures for integration testing
 from importlib.metadata import version
+from pathlib import Path
 
 import pytest
 
 __version__: str = version("pytest-simcore")
 
 
 def pytest_addoption(parser):
-    group = parser.getgroup("simcore")
-    group.addoption(
+    simcore_group = parser.getgroup(
+        "simcore", description="options related to pytest simcore"
+    )
+    simcore_group.addoption(
         "--keep-docker-up",
         action="store_true",
         default=False,
         help="Keep stack/registry up after fixtures closes",
     )
 
+    simcore_group.addoption(
+        "--external-envfile",
+        action="store",
+        type=Path,
+        default=None,
+        help="Path to an env file. Consider passing a link to repo configs, i.e. `ln -s /path/to/osparc-ops-config/repo.config`",
+    )
+
     # DUMMY
     parser.addini("HELLO", "Dummy pytest.ini setting")
 

packages/pytest-simcore/src/pytest_simcore/docker_registry.py

@@ -16,6 +16,7 @@
 import jsonschema
 import pytest
 import tenacity
+from pytest_simcore.helpers.typing_env import EnvVarsDict
 from pytest_simcore.logging_utils import log_context
 from settings_library.docker_registry import RegistrySettings
 
@@ -99,7 +100,24 @@ def docker_registry(keep_docker_up: bool) -> Iterator[str]:
 
 
 @pytest.fixture
-def registry_settings(docker_registry: str) -> RegistrySettings:
+def external_registry_settings(
+    external_environment: EnvVarsDict,
+) -> RegistrySettings | None:
+    if external_environment:
+        config = {
+            field: external_environment.get(field, None)
+            for field in RegistrySettings.__fields__
+        }
+        return RegistrySettings.parse_obj(config)
+    return None
+
+
+@pytest.fixture
+def registry_settings(
+    docker_registry: str, external_registry_settings: RegistrySettings | None
+) -> RegistrySettings:
+    if external_registry_settings:
+        return external_registry_settings
     return RegistrySettings.create_from_envs()
 
 

packages/pytest-simcore/src/pytest_simcore/environment_configs.py

@@ -36,3 +36,24 @@ def all_env_devel_undefined(
     when some script was accidentaly injecting the entire .env-devel in the environment
     """
     delenvs_from_dict(monkeypatch, env_devel_dict, raising=False)
+
+
+@pytest.fixture(scope="session")
+def external_environment(request: pytest.FixtureRequest) -> EnvVarsDict:
+    """
+    If a file under test folder prefixed with `.env-secret` is present,
+    then this fixture captures it.
+
+    This technique allows reusing the same tests to check against
+    external development/production servers
+    """
+    envs = {}
+    if envfile := request.config.getoption("--external-envfile"):
+        print("🚨 EXTERNAL `envfile` option detected. Loading", envfile, "...")
+
+        assert isinstance(envfile, Path)
+        assert envfile.is_file()
+
+        envs = load_dotenv(envfile)
+
+    return envs

packages/service-library/src/servicelib/docker_utils.py

@@ -3,6 +3,7 @@
 from contextlib import AsyncExitStack
 from dataclasses import dataclass
 from datetime import datetime
+from functools import cached_property
 from typing import Any, Final, Literal
 
 import aiodocker
@@ -52,10 +53,15 @@ class DockerImageManifestsV2(BaseModel):
     layers: list[DockerLayerSizeV2]
 
     class Config:
+        keep_untouched = (cached_property,)
         frozen = True
         alias_generator = snake_to_camel
         allow_population_by_field_name = True
 
+    @cached_property
+    def layers_total_size(self) -> ByteSize:
+        return parse_obj_as(ByteSize, sum(layer.size for layer in self.layers))
+
 
 class DockerImageMultiArchManifestsV2(BaseModel):
     schema_version: Literal[2]
@@ -161,11 +167,8 @@ async def pull_image(
                 layer.digest.removeprefix("sha256:")[:12]: _PulledStatus(layer.size)
                 for layer in image_information.layers
             }
-            image_layers_total_size = (
-                sum(layer.size for layer in image_information.layers) * 3
-            )
             sub_progress = await exit_stack.enter_async_context(
-                progress_bar.sub_progress(image_layers_total_size)
+                progress_bar.sub_progress(image_information.layers_total_size * 3)
             )
         else:
             _logger.warning(

packages/service-library/src/servicelib/fastapi/docker_utils.py

@@ -1,8 +1,10 @@
+import asyncio
 import logging
+from typing import Final
 
 import httpx
 from models_library.docker import DockerGenericTag
-from pydantic import ValidationError, parse_obj_as
+from pydantic import ByteSize, ValidationError, parse_obj_as
 from settings_library.docker_registry import RegistrySettings
 from yarl import URL
 
@@ -11,10 +13,13 @@
     DOCKER_HUB_HOST,
     DockerImageManifestsV2,
     DockerImageMultiArchManifestsV2,
+    LogCB,
     get_image_complete_url,
     get_image_name_and_tag,
+    pull_image,
 )
 from ..logging_utils import log_catch
+from ..progress_bar import AsyncReportCB, ProgressBarData
 
 _logger = logging.getLogger(__name__)
 
@@ -92,3 +97,48 @@ async def retrieve_image_layer_information(
             except ValidationError:
                 return parse_obj_as(DockerImageManifestsV2, json_response)
     return None
+
+
+_DEFAULT_MIN_IMAGE_SIZE: Final[ByteSize] = parse_obj_as(ByteSize, "50MiB")
+
+
+async def pull_images(
+    images: set[DockerGenericTag],
+    registry_settings: RegistrySettings,
+    progress_cb: AsyncReportCB,
+    log_cb: LogCB,
+) -> None:
+    images_layer_information = await asyncio.gather(
+        *[
+            retrieve_image_layer_information(image, registry_settings)
+            for image in images
+        ]
+    )
+    progress_step_weights = [
+        float(i.layers_total_size) if i else float(_DEFAULT_MIN_IMAGE_SIZE)
+        for i in images_layer_information
+    ]
+    _logger.debug("images to pull sizes: %s", progress_step_weights)
+
+    async with ProgressBarData(
+        num_steps=len(images),
+        step_weights=progress_step_weights,
+        progress_report_cb=progress_cb,
+    ) as pbar:
+        for image, image_layer_info in zip(
+            images, images_layer_information, strict=True
+        ):
+            # NOTE: use gather call here to pull faster
+            # problem with progress in concurrent calls, needs to fix the progress bar
+
+            await pull_image(
+                image,
+                registry_settings,
+                pbar,
+                log_cb,
+                (
+                    image_layer_info
+                    if isinstance(image_layer_info, DockerImageManifestsV2)
+                    else None
+                ),
+            )

packages/service-library/src/servicelib/progress_bar.py

@@ -6,8 +6,9 @@
 
 from .logging_utils import log_catch
 
-logger = logging.getLogger(__name__)
+_logger = logging.getLogger(__name__)
 _MIN_PROGRESS_UPDATE_PERCENT: Final[float] = 0.01
+_INITIAL_VALUE: Final[float] = -1.0
 _FINAL_VALUE: Final[float] = 1.0
 
 
@@ -30,9 +31,6 @@ def _normalize_weights(steps: int, weights: list[float]) -> list[float]:
     return [weight / total for weight in weights]
 
 
-_INITIAL_VALUE: Final[float] = -1
-
-
 @dataclass(slots=True, kw_only=True)
 class ProgressBarData:
     """A progress bar data allows to keep track of multiple progress(es) even in deeply nested processes.
@@ -107,7 +105,7 @@ async def _report_external(self, value: float, *, force: bool = False) -> None:
         if not self.progress_report_cb:
             return
 
-        with log_catch(logger, reraise=False):
+        with log_catch(_logger, reraise=False):
             # NOTE: only report if at least a percent was increased
             if (
                 (force and value != self._last_report_value)
@@ -138,7 +136,7 @@ async def update(self, steps: float = 1) -> None:
             if new_steps_value > self.num_steps:
                 new_steps_value = round(new_steps_value)
             if new_steps_value > self.num_steps:
-                logger.warning(
+                _logger.warning(
                     "%s",
                     f"Progress already reached maximum of {self.num_steps=}, "
                     f"cause: {self._current_steps=} is updated by {steps=}"
@@ -148,6 +146,9 @@ async def update(self, steps: float = 1) -> None:
 
                 new_steps_value = self.num_steps
 
+            if new_steps_value == self._current_steps:
+                return
+
             new_progress_value = self._compute_progress(new_steps_value)
             if self._current_steps != _INITIAL_VALUE:
                 old_progress_value = self._compute_progress(self._current_steps)
@@ -159,10 +160,10 @@ async def update(self, steps: float = 1) -> None:
         await self._report_external(new_progress_value)
 
     async def set_(self, new_value: float) -> None:
-        if update_value := round(new_value - self._current_steps):
-            await self.update(update_value)
+        await self.update(new_value - self._current_steps)
 
     async def finish(self) -> None:
+        _logger.debug("finishing %s", f"{self.num_steps} progress")
         await self.set_(self.num_steps)
 
     def sub_progress(

packages/service-library/tests/conftest.py

@@ -15,6 +15,7 @@
     "pytest_simcore.docker_compose",
     "pytest_simcore.docker_registry",
     "pytest_simcore.docker_swarm",
+    "pytest_simcore.environment_configs",
     "pytest_simcore.file_extra",
     "pytest_simcore.pytest_global_environs",
     "pytest_simcore.rabbit_service",

packages/service-library/tests/fastapi/test_docker_utils.py

@@ -9,7 +9,10 @@
 from pytest_mock import MockerFixture
 from servicelib import progress_bar
 from servicelib.docker_utils import pull_image
-from servicelib.fastapi.docker_utils import retrieve_image_layer_information
+from servicelib.fastapi.docker_utils import (
+    pull_images,
+    retrieve_image_layer_information,
+)
 from settings_library.docker_registry import RegistrySettings
 
 
@@ -120,3 +123,39 @@ async def _log_cb(*args, **kwargs) -> None:
     # check there were no warnings
     for record in caplog.records:
         assert record.levelname != "WARNING", record.message
+
+
+@pytest.mark.parametrize(
+    "images_set",
+    [
+        {"itisfoundation/sleeper:1.0.0", "nginx:latest", "busybox:latest"},
+    ],
+)
+async def test_pull_images(
+    remove_images_from_host: Callable[[list[str]], Awaitable[None]],
+    images_set: set[DockerGenericTag],
+    registry_settings: RegistrySettings,
+    mocker: MockerFixture,
+    caplog: pytest.LogCaptureFixture,
+):
+    await remove_images_from_host(list(images_set))
+
+    async def _log_cb(*args, **kwargs) -> None:
+        print(f"received log: {args}, {kwargs}")
+
+    async def _progress_cb(*args, **kwargs) -> None:
+        print(f"received progress: {args}, {kwargs}")
+
+    fake_progress_report_cb = mocker.AsyncMock(side_effect=_progress_cb)
+    fake_log_cb = mocker.AsyncMock(side_effect=_log_cb)
+    await pull_images(
+        images_set, registry_settings, fake_progress_report_cb, fake_log_cb
+    )
+    fake_log_cb.assert_called()
+    assert fake_progress_report_cb.call_args_list[0] == call(0.0)
+    assert fake_progress_report_cb.call_args_list[-1] == call(1.0)
+    fake_progress_report_cb.reset_mock()
+
+    # check there were no warnings
+    # NOTE: this would pop up in case docker changes its pulling statuses
+    assert not [r.message for r in caplog.records if r.levelname == "WARNING"]