Skip to content

Commit

Permalink
Discourage use of @ in layer names
Browse files Browse the repository at this point in the history
* Updated docs to actively discourage using ``@`` in layers names
* Rename "explicitly versioned" to "externally versioned", and make it
  clear that is a separate concept from implicit layer lock versioning
* Rename `fully_versioned_name` runtime layer spec field to `implementation_name`
* Simplified ``runtime_name`` in layer metadata to always refer to the runtime install target
* Added `implementation_name` to the published layer metadata
* Added `bound_to_implementation` to the published layer metadata
* Add mechanism to emit `FutureWarning` for renamed and removed spec
  fields (which still accepting them)

Continues work on #78
  • Loading branch information
ncoghlan committed Nov 12, 2024
1 parent 7877f74 commit 9570404
Show file tree
Hide file tree
Showing 72 changed files with 617 additions and 512 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
.. Included in published docs via docs/changelog.rst
.. Temporary link target for next release
.. _changelog-0.2.0:

Unreleased
==========

Expand Down
1 change: 0 additions & 1 deletion docs/api/stacks/venvstacks.stacks.ApplicationSpec.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,5 @@ venvstacks.stacks.ApplicationSpec
~ApplicationSpec.name
~ApplicationSpec.versioned
~ApplicationSpec.requirements
~ApplicationSpec.build_requirements
~ApplicationSpec.platforms

1 change: 0 additions & 1 deletion docs/api/stacks/venvstacks.stacks.FrameworkSpec.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,5 @@ venvstacks.stacks.FrameworkSpec
~FrameworkSpec.name
~FrameworkSpec.versioned
~FrameworkSpec.requirements
~FrameworkSpec.build_requirements
~FrameworkSpec.platforms

1 change: 0 additions & 1 deletion docs/api/stacks/venvstacks.stacks.LayerSpecBase.rst
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,5 @@ venvstacks.stacks.LayerSpecBase
~LayerSpecBase.name
~LayerSpecBase.versioned
~LayerSpecBase.requirements
~LayerSpecBase.build_requirements
~LayerSpecBase.platforms

1 change: 0 additions & 1 deletion docs/api/stacks/venvstacks.stacks.LayeredSpecBase.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,5 @@ venvstacks.stacks.LayeredSpecBase
~LayeredSpecBase.name
~LayeredSpecBase.versioned
~LayeredSpecBase.requirements
~LayeredSpecBase.build_requirements
~LayeredSpecBase.platforms

3 changes: 1 addition & 2 deletions docs/api/stacks/venvstacks.stacks.RuntimeSpec.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ venvstacks.stacks.RuntimeSpec
~RuntimeSpec.env_name
~RuntimeSpec.kind
~RuntimeSpec.py_version
~RuntimeSpec.fully_versioned_name
~RuntimeSpec.implementation_name
~RuntimeSpec.name
~RuntimeSpec.versioned
~RuntimeSpec.requirements
~RuntimeSpec.build_requirements
~RuntimeSpec.platforms

Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Added
-----

- Added documentation for the :ref:`stack-specification-format`.
- Added documentation for the :ref:`stack-specification-format` (added in :issue:`78`).
- Added ``implementation_name`` to the published layer metadata (added in :issue:`78`).
- Added ``bound_to_implementation`` to the published layer metadata (added in :issue:`78`).

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Changed
-------

- Updated docs to actively discourage using ``@`` in layers names (part of :issue:`78`).
- Renamed ``fully_versioned_name`` runtime layer specification field to ``implementation_name`` (part of :issue:`78`).
- Simplified ``runtime_name`` in layer metadata to always refer to the runtime install target (part of :issue:`78`).

215 changes: 159 additions & 56 deletions docs/file-formats.rst

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions docs/overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ all running in a controlled Python 3.11 base runtime:
.. code-block:: toml
[[runtimes]]
name = "cpython@3.11"
fully_versioned_name = "[email protected]"
name = "cpython-3.11"
implementation_name = "[email protected]"
requirements = [
"numpy",
]
[[frameworks]]
name = "sklearn"
runtime = "cpython@3.11"
runtime = "cpython-3.11"
requirements = [
"scikit-learn",
]
Expand Down
2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ markers = [
"slow: marks tests as slow (deselect with '-m \"not slow\"')",
"expected_output: tests to run when regenerating expected output",
]
# Warnings should only be emitted when being specifically tested
filterwarnings = ["error"]
# Make long diffs visible in pytest 8.3.3 and later
verbosity_assertions = 2
# Ensure test suite doesn't consume too much space in /tmp, while still allowing debugging
Expand Down
131 changes: 107 additions & 24 deletions src/venvstacks/stacks.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import sysconfig
import tempfile
import tomllib
import warnings

from abc import ABC, abstractmethod
from dataclasses import dataclass, field, InitVar
Expand Down Expand Up @@ -411,7 +412,6 @@ class LayerCategories(StrEnum):
def ensure_optional_env_spec_fields(env_metadata: MutableMapping[str, Any]) -> None:
"""Populate missing environment spec fields that are optional in the TOML file."""
TargetPlatforms.ensure_platform_list(env_metadata)
env_metadata.setdefault("build_requirements", [])
env_metadata.setdefault("versioned", False)


Expand All @@ -430,7 +430,6 @@ class LayerSpecBase(ABC):
name: LayerBaseName
versioned: bool
requirements: list[str] = field(repr=False)
build_requirements: list[str] = field(repr=False)
platforms: list[TargetPlatforms] = field(repr=False)

def __post_init__(self) -> None:
Expand Down Expand Up @@ -470,19 +469,20 @@ class RuntimeSpec(LayerSpecBase):

kind = LayerVariants.RUNTIME
category = LayerCategories.RUNTIMES
fully_versioned_name: str = field(repr=False)
implementation_name: str = field(repr=False)

@property
def py_version(self) -> str:
"""Extract just the Python version string from the base runtime identifier."""
# fully_versioned_name should be of the form "[email protected]"
# implementation_name should be of the form "[email protected]"
# (this may need adjusting if runtimes other than CPython are ever used...)
return self.fully_versioned_name.partition("@")[2]
return self.implementation_name.partition("@")[2]


@dataclass
class LayeredSpecBase(LayerSpecBase):
"""Common base class for framework and application layer specifications."""

# Intermediate class for covariant property typing (never instantiated)
runtime: RuntimeSpec = field(repr=False)

Expand Down Expand Up @@ -518,23 +518,30 @@ class LayerSpecMetadata(TypedDict):
lock_version: int # Monotonically increasing version identifier
locked_at: str # ISO formatted date/time value

# Extra fields only defined for framework and application environments
# runtime_name is set to fully_versioned_name if maintenance updates trigger a rebuild,
# otherwise set to runtime's layer spec name so only feature releases force a rebuild
# Fields that are populated after the layer metadata has initially been defined
# "runtime_name" is set to the underlying runtime's deployed environment name
# "implementation_name" is set to the underlying runtime's implementation name
# "bound_to_implementation" means that the layered environment includes
# copies of some files from the runtime implementation, and hence will
# need updating even for runtime maintenance releases
runtime_name: NotRequired[str]
implementation_name: NotRequired[str]
bound_to_implementation: NotRequired[bool]

# Extra fields only defined for framework and application environments
required_layers: NotRequired[Sequence[EnvNameDeploy]]

# Extra fields only defined for application environments
app_launch_module: NotRequired[str]
app_launch_module_hash: NotRequired[str]
# fmt: on

# Note: hashes of layered environment dependencies are intentionally NOT incorporated
# into the published metadata. This allows an "only if needed" approach to
# rebuilding app and framework layers when the layers they depend on are
# updated (app layers will usually only depend on some of the components in the
# underlying environment, and such dependencies are picked up as version changes
# when regenerating the transitive dependency specifications for each environment)
# fmt: on


######################################################
Expand Down Expand Up @@ -902,7 +909,8 @@ def _pdm_python_install(target_path: Path, request: str) -> Path | None:
destination.mkdir(parents=True, exist_ok=True)
with tempfile.NamedTemporaryFile() as tf:
tf.close()
original_filename = download(python_file, tf.name, env.session)
with env.session:
original_filename = download(python_file, tf.name, env.session)
# Use "tar_filter" if stdlib tar extraction filters are available
# (they were only added in Python 3.12, so no filtering on 3.11)
with default_tarfile_filter("tar_filter"):
Expand Down Expand Up @@ -985,6 +993,7 @@ def get_build_platform() -> TargetPlatform:
@dataclass
class LayerEnvBase(ABC):
"""Common base class for layer build environment implementations."""

# Python environment used to run tools like `uv` and `pip`
tools_python_path: ClassVar[Path] = Path(sys.executable)

Expand Down Expand Up @@ -1505,7 +1514,7 @@ def _remove_pip(self) -> subprocess.CompletedProcess[str] | None:
return self._run_pip(pip_args)

def _create_new_environment(self, *, lock_only: bool = False) -> None:
python_runtime = self.env_spec.fully_versioned_name
python_runtime = self.env_spec.implementation_name
install_path = _pdm_python_install(self.build_path, python_runtime)
if install_path is None:
self._fail_build(f"Failed to install {python_runtime}")
Expand All @@ -1524,7 +1533,9 @@ def _create_new_environment(self, *, lock_only: bool = False) -> None:
def _update_output_metadata(self, metadata: LayerSpecMetadata) -> None:
super()._update_output_metadata(metadata)
# This *is* a runtime layer, so it needs to be updated on maintenance releases
metadata["runtime_name"] = self.env_spec.fully_versioned_name
metadata["runtime_name"] = self.install_target
metadata["implementation_name"] = self.env_spec.implementation_name
metadata["bound_to_implementation"] = True

def create_build_environment(self, *, clean: bool = False) -> None:
"""Create or update runtime build environment. Returns True if env is new or updated."""
Expand All @@ -1533,6 +1544,7 @@ def create_build_environment(self, *, clean: bool = False) -> None:

class LayeredEnvBase(LayerEnvBase):
"""Common base class for framework and application layer build environments."""

base_runtime: RuntimeEnv | None = field(init=False, repr=False)
linked_constraints_paths: list[Path] = field(init=False, repr=False)

Expand Down Expand Up @@ -1660,11 +1672,11 @@ def _update_output_metadata(self, metadata: LayerSpecMetadata) -> None:
super()._update_output_metadata(metadata)
# Non-windows platforms use symlinks, so only need updates on feature releases
# Windows copies the main Python binary and support libary, so always needs updates
if _WINDOWS_BUILD:
runtime_update_trigger = self.env_spec.runtime.fully_versioned_name
else:
runtime_update_trigger = self.env_spec.runtime.name
metadata["runtime_name"] = runtime_update_trigger
runtime = self.base_runtime
assert runtime is not None
metadata["runtime_name"] = runtime.install_target
metadata["implementation_name"] = runtime.env_spec.implementation_name
metadata["bound_to_implementation"] = bool(_WINDOWS_BUILD)


class FrameworkEnv(LayeredEnvBase):
Expand Down Expand Up @@ -1783,6 +1795,69 @@ def __post_init__(self) -> None:
self.requirements_dir_path
)

@staticmethod
def _get_layer_name(data: Mapping[str, Any]) -> Any:
try:
return data["name"]
except KeyError:
pass # This error context is not interesting
raise LayerSpecError("Layer specifications must include 'name'")

@classmethod
def _delete_field(cls, data: MutableMapping[str, Any], legacy_name: str) -> bool:
"""Ignore removed legacy field. Returns True if field needs to be removed."""
legacy_field_value = data.pop(legacy_name, None)
if legacy_field_value is not None:
layer_name = cls._get_layer_name(data)
msg = f"Dropping legacy field {legacy_name!r} for layer {layer_name!r}"
warnings.warn(msg, FutureWarning)
return True
return False

@classmethod
def _update_field_name(
cls, data: MutableMapping[str, Any], legacy_name: str, name: str
) -> bool:
"""Convert legacy field to current field. Returns True if conversion is needed."""
legacy_field_value = data.pop(legacy_name, None)
if legacy_field_value is not None:
layer_name = cls._get_layer_name(data)
if name in data:
msg = f"Layer {layer_name!r} sets both {name!r} and the obsolete {legacy_name!r}"
raise LayerSpecError(msg)
data[name] = legacy_field_value
msg = f"Converting legacy field name {legacy_name!r} to {name!r} for layer {layer_name!r}"
warnings.warn(msg, FutureWarning)
return True
return False

@classmethod
def _update_legacy_fields(
cls,
data: MutableMapping[str, Any],
conversions: Mapping[str, str | None],
) -> bool:
modified = False
for legacy_name, name in conversions.items():
if name is None:
field_modified = cls._delete_field(data, legacy_name)
else:
field_modified = cls._update_field_name(data, legacy_name, name)
if field_modified:
modified = True
return modified

_RUNTIME_LEGACY_CONVERSIONS: ClassVar[Mapping[str, str | None]] = {
"fully_versioned_name": "implementation_name",
"build_requirements": None,
}
_FRAMEWORK_LEGACY_CONVERSIONS: ClassVar[Mapping[str, str | None]] = {
"build_requirements": None,
}
_APPLICATION_LEGACY_CONVERSIONS: ClassVar[Mapping[str, str | None]] = {
"build_requirements": None,
}

@classmethod
def load(cls, fname: StrPath) -> Self:
"""Load stack specification from given TOML file."""
Expand All @@ -1793,18 +1868,23 @@ def load(cls, fname: StrPath) -> Self:
requirements_dir_path = spec_dir_path / "requirements"
# Collect the list of runtime specs
runtimes = {}
for rt in data["runtimes"]:
# No conversions needed for the runtime environment specs
name = rt["name"]
for rt in data.get("runtimes", ()):
name = cls._get_layer_name(rt)
# Handle backwards compatibility fixes and warnings
cls._update_legacy_fields(rt, cls._RUNTIME_LEGACY_CONVERSIONS)
# Consistency checks (no field value conversions necessary)
if name in runtimes:
msg = f"Runtime names must be distinct ({name!r} already defined)"
raise LayerSpecError(msg)
ensure_optional_env_spec_fields(rt)
runtimes[name] = RuntimeSpec(**rt)
# Collect the list of framework specs
frameworks = {}
for fw in data["frameworks"]:
name = fw["name"]
for fw in data.get("frameworks", ()):
name = cls._get_layer_name(fw)
# Handle backwards compatibility fixes and warnings
cls._update_legacy_fields(fw, cls._FRAMEWORK_LEGACY_CONVERSIONS)
# Consistency checks and field value conversions
if name in frameworks:
msg = f"Framework names must be distinct ({name!r} already defined)"
raise LayerSpecError(msg)
Expand All @@ -1818,8 +1898,11 @@ def load(cls, fname: StrPath) -> Self:
frameworks[name] = FrameworkSpec(**fw)
# Collect the list of application specs
applications = {}
for app in data["applications"]:
name = app["name"]
for app in data.get("applications", ()):
name = cls._get_layer_name(app)
# Handle backwards compatibility fixes and warnings
cls._update_legacy_fields(app, cls._APPLICATION_LEGACY_CONVERSIONS)
# Consistency checks and field value conversions
if name in applications:
msg = f"Application names must be distinct ({name!r} already defined)"
raise LayerSpecError(msg)
Expand Down
7 changes: 3 additions & 4 deletions tests/minimal_project/venvstacks.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@
# actual content of the layers doesn't matter, the environments just need to exist.

[[runtimes]]
name = "cpython@3.11"
fully_versioned_name = "[email protected]"
name = "cpython-3.11"
implementation_name = "[email protected]"
requirements = []
build_requirements = []

[[frameworks]]
name = "layer"
runtime = "cpython@3.11"
runtime = "cpython-3.11"
requirements = []

[[applications]]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
"app_launch_module_hash": "sha256/76d203dd6d3bb1fdb1117053c81590d42ef3f62b4b92b8d316b31f08334fbca6",
"archive_build": 1,
"archive_hashes": {
"sha256": "e114727c3657844ea74de25009748a0c90b7ccf1279a787a9c8f5dd9cdd4e020"
"sha256": "45c0540c1b4e9a9afbf2c7f5ade7a301ada4861b189a80c5933d96e07fe6cd1f"
},
"archive_name": "app-scipy-client.tar.xz",
"archive_size": 3008,
"bound_to_implementation": false,
"implementation_name": "[email protected]",
"install_target": "app-scipy-client",
"layer_name": "app-scipy-client",
"lock_version": 1,
Expand All @@ -16,6 +18,6 @@
"framework-http-client"
],
"requirements_hash": "sha256:fb8a843c694d03d7ee74b457cdac2bd82b6b439de0ed308d72fe698c6c9c6cf4",
"runtime_name": "cpython@3.11",
"runtime_name": "cpython-3.11",
"target_platform": "linux_x86_64"
}
Loading

0 comments on commit 9570404

Please sign in to comment.