Skip to content

Commit

Permalink
Start phasing out frozen_after_init (pantsbuild#18047)
Browse files Browse the repository at this point in the history
As discussed on Slack, `frozen_after_init` has a few drawbacks:

- We lose the normal "frozen" benefits from dataclass
- You have to remember to use `unsafe_hash`, because...
- Type checkers won't help you identify when assign a variable. You're on your own
- These are mostly used for converters. If/when those get into stdlib `dataclass` we could eliminate a lot of this code.

On the other hand, the standard library already spells out what to do in this situation: https://docs.python.org/3/library/dataclasses.html#frozen-instances. I love doing as the Romans do 😉 

After we switch internally, we can deprecate the function for a few releases
  • Loading branch information
thejcannon authored Jan 20, 2023
1 parent 5f5fe87 commit a3d9e25
Show file tree
Hide file tree
Showing 17 changed files with 231 additions and 161 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,16 @@
from pants.jvm.resolve.coursier_fetch import CoursierResolvedLockfile
from pants.jvm.resolve.lockfile_metadata import LockfileContext
from pants.util.docutil import bin_name
from pants.util.meta import frozen_after_init


@frozen_after_init
@dataclass(unsafe_hash=True)
@dataclass(frozen=True)
class JVMLockfileFixtureDefinition:
lockfile_rel_path: Path
requirements: tuple[Coordinate, ...]

def __init__(
self, lockfile_rel_path: Path | str, requirements: Iterable[Coordinate | str]
) -> None:
self.lockfile_rel_path = (
lockfile_rel_path if isinstance(lockfile_rel_path, Path) else Path(lockfile_rel_path)
)

coordinates: list[Coordinate] = []
for requirement in requirements:
if isinstance(requirement, Coordinate):
Expand All @@ -40,7 +34,13 @@ def __init__(
raise TypeError(
f"Unsupported type `{type(requirement)}` for JVM coordinate. Expected `Coordinate` or `str`."
)
self.requirements = tuple(coordinates)

object.__setattr__(
self,
"lockfile_rel_path",
lockfile_rel_path if isinstance(lockfile_rel_path, Path) else Path(lockfile_rel_path),
)
object.__setattr__(self, "requirements", tuple(coordinates))

@classmethod
def from_json_dict(cls, kwargs) -> JVMLockfileFixtureDefinition:
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ python_sources(
)

python_test_utils(name="test_utils")
python_tests(name="tests")

python_distribution(
name="pants-packaged",
Expand Down
8 changes: 3 additions & 5 deletions src/python/pants/backend/go/util_rules/embedcfg.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@
from typing import Any, Iterable, Mapping

from pants.util.frozendict import FrozenDict
from pants.util.meta import frozen_after_init
from pants.util.strutil import strip_prefix


@dataclass(unsafe_hash=True)
@frozen_after_init
@dataclass(frozen=True)
class EmbedConfig:
patterns: FrozenDict[str, tuple[str, ...]]
files: FrozenDict[str, str]
Expand All @@ -35,8 +33,8 @@ def __init__(self, patterns: Mapping[str, Iterable[str]], files: Mapping[str, st
:param files: Maps each virtual, relative file path used as a value in the `patterns`
dictionary to the actual filesystem path with that file's content.
"""
self.patterns = FrozenDict({k: tuple(v) for k, v in patterns.items()})
self.files = FrozenDict(files)
object.__setattr__(self, "patterns", FrozenDict({k: tuple(v) for k, v in patterns.items()}))
object.__setattr__(self, "files", FrozenDict(files))

@classmethod
def from_json_dict(
Expand Down
30 changes: 16 additions & 14 deletions src/python/pants/backend/go/util_rules/sdk.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,9 @@
from pants.engine.rules import collect_rules, rule
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.meta import frozen_after_init


@frozen_after_init
@dataclass(unsafe_hash=True)
@dataclass(frozen=True)
class GoSdkProcess:
command: tuple[str, ...]
description: str
Expand All @@ -46,18 +44,22 @@ def __init__(
allow_downloads: bool = False,
replace_sandbox_root_in_args: bool = False,
) -> None:
self.command = tuple(command)
self.description = description
self.env = (
FrozenDict(env or {})
if allow_downloads
else FrozenDict({**(env or {}), "GOPROXY": "off"})
object.__setattr__(self, "command", tuple(command))
object.__setattr__(self, "description", description)
object.__setattr__(
self,
"env",
(
FrozenDict(env or {})
if allow_downloads
else FrozenDict({**(env or {}), "GOPROXY": "off"})
),
)
self.input_digest = input_digest
self.working_dir = working_dir
self.output_files = tuple(output_files)
self.output_directories = tuple(output_directories)
self.replace_sandbox_root_in_args = replace_sandbox_root_in_args
object.__setattr__(self, "input_digest", input_digest)
object.__setattr__(self, "working_dir", working_dir)
object.__setattr__(self, "output_files", tuple(output_files))
object.__setattr__(self, "output_directories", tuple(output_directories))
object.__setattr__(self, "replace_sandbox_root_in_args", replace_sandbox_root_in_args)


@dataclass(frozen=True)
Expand Down
18 changes: 9 additions & 9 deletions src/python/pants/backend/helm/subsystems/post_renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
from pants.util.docutil import git_url
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.meta import frozen_after_init
from pants.util.strutil import bullet_list, pluralize, softwrap

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -126,8 +125,7 @@ def debug_hint(self) -> str | None:
return self.description_of_origin


@frozen_after_init
@dataclass(unsafe_hash=True)
@dataclass(frozen=True)
class HelmPostRenderer(EngineAwareReturnType):
exe: str
digest: Digest
Expand All @@ -146,12 +144,14 @@ def __init__(
immutable_input_digests: Mapping[str, Digest] | None = None,
append_only_caches: Mapping[str, str] | None = None,
) -> None:
self.exe = exe
self.digest = digest
self.description_of_origin = description_of_origin
self.env = FrozenDict(env or {})
self.append_only_caches = FrozenDict(append_only_caches or {})
self.immutable_input_digests = FrozenDict(immutable_input_digests or {})
object.__setattr__(self, "exe", exe)
object.__setattr__(self, "digest", digest)
object.__setattr__(self, "description_of_origin", description_of_origin)
object.__setattr__(self, "env", FrozenDict(env or {}))
object.__setattr__(self, "append_only_caches", FrozenDict(append_only_caches or {}))
object.__setattr__(
self, "immutable_input_digests", FrozenDict(immutable_input_digests or {})
)

def level(self) -> LogLevel | None:
return LogLevel.DEBUG
Expand Down
14 changes: 6 additions & 8 deletions src/python/pants/backend/helm/util_rules/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
from pants.engine.process import InteractiveProcess, Process, ProcessCacheScope, ProcessResult
from pants.engine.rules import Get, MultiGet, collect_rules, rule, rule_helper
from pants.util.logging import LogLevel
from pants.util.meta import frozen_after_init
from pants.util.strutil import pluralize, softwrap
from pants.util.value_interpolation import InterpolationContext, InterpolationValue

Expand All @@ -56,8 +55,7 @@ class HelmDeploymentCmd(Enum):
RENDER = "template"


@dataclass(unsafe_hash=True)
@frozen_after_init
@dataclass(frozen=True)
class HelmDeploymentRequest(EngineAwareParameter):
field_set: HelmDeploymentFieldSet

Expand All @@ -75,11 +73,11 @@ def __init__(
extra_argv: Iterable[str] | None = None,
post_renderer: HelmPostRenderer | None = None,
) -> None:
self.field_set = field_set
self.cmd = cmd
self.description = description
self.extra_argv = tuple(extra_argv or ())
self.post_renderer = post_renderer
object.__setattr__(self, "field_set", field_set)
object.__setattr__(self, "cmd", cmd)
object.__setattr__(self, "description", description)
object.__setattr__(self, "extra_argv", tuple(extra_argv or ()))
object.__setattr__(self, "post_renderer", post_renderer)

def debug_hint(self) -> str | None:
return self.field_set.address.spec
Expand Down
39 changes: 20 additions & 19 deletions src/python/pants/backend/helm/util_rules/tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
from pants.option.subsystem import Subsystem
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.meta import frozen_after_init
from pants.util.strutil import bullet_list, pluralize

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -285,8 +284,7 @@ async def download_external_helm_plugin(request: ExternalHelmPluginRequest) -> H
# ---------------------------------------------


@frozen_after_init
@dataclass(unsafe_hash=True)
@dataclass(frozen=True)
class HelmBinary:
path: str

Expand All @@ -301,9 +299,9 @@ def __init__(
local_env: Mapping[str, str],
immutable_input_digests: Mapping[str, Digest],
) -> None:
self.path = path
self.immutable_input_digests = FrozenDict(immutable_input_digests)
self.env = FrozenDict({**helm_env, **local_env})
object.__setattr__(self, "path", path)
object.__setattr__(self, "immutable_input_digests", FrozenDict(immutable_input_digests))
object.__setattr__(self, "env", FrozenDict({**helm_env, **local_env}))

@property
def config_digest(self) -> Digest:
Expand All @@ -318,8 +316,7 @@ def append_only_caches(self) -> dict[str, str]:
return {_HELM_CACHE_NAME: _HELM_CACHE_DIR}


@frozen_after_init
@dataclass(unsafe_hash=True)
@dataclass(frozen=True)
class HelmProcess:
argv: tuple[str, ...]
input_digest: Digest
Expand Down Expand Up @@ -348,17 +345,21 @@ def __init__(
cache_scope: ProcessCacheScope | None = None,
timeout_seconds: int | None = None,
):
self.argv = tuple(argv)
self.input_digest = input_digest
self.description = description
self.level = level
self.output_directories = tuple(output_directories or ())
self.output_files = tuple(output_files or ())
self.extra_env = FrozenDict(extra_env or {})
self.extra_immutable_input_digests = FrozenDict(extra_immutable_input_digests or {})
self.extra_append_only_caches = FrozenDict(extra_append_only_caches or {})
self.cache_scope = cache_scope
self.timeout_seconds = timeout_seconds
object.__setattr__(self, "argv", tuple(argv))
object.__setattr__(self, "input_digest", input_digest)
object.__setattr__(self, "description", description)
object.__setattr__(self, "level", level)
object.__setattr__(self, "output_directories", tuple(output_directories or ()))
object.__setattr__(self, "output_files", tuple(output_files or ()))
object.__setattr__(self, "extra_env", FrozenDict(extra_env or {}))
object.__setattr__(
self, "extra_immutable_input_digests", FrozenDict(extra_immutable_input_digests or {})
)
object.__setattr__(
self, "extra_append_only_caches", FrozenDict(extra_append_only_caches or {})
)
object.__setattr__(self, "cache_scope", cache_scope)
object.__setattr__(self, "timeout_seconds", timeout_seconds)


@rule(desc="Download and configure Helm", level=LogLevel.DEBUG)
Expand Down
10 changes: 4 additions & 6 deletions src/python/pants/backend/helm/utils/yaml.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,9 @@

from pants.engine.collection import Collection
from pants.util.frozendict import FrozenDict
from pants.util.meta import frozen_after_init


@dataclass(unsafe_hash=True)
@frozen_after_init
class YamlPath:
"""Simple implementation of YAML paths using `/` syntax and being the single slash the path to
the root."""
Expand All @@ -24,8 +22,8 @@ class YamlPath:
_absolute: bool

def __init__(self, elements: Iterable[str], *, absolute: bool) -> None:
self._elements = tuple(elements)
self._absolute = absolute
object.__setattr__(self, "_elements", tuple(elements))
object.__setattr__(self, "_absolute", absolute)

if len(self._elements) == 0 and not self._absolute:
raise ValueError("Relative YAML paths with no elements are not allowed.")
Expand Down Expand Up @@ -181,7 +179,7 @@ def to_json_dict(self) -> dict[str, dict[str, str]]:
return {"paths": items_dict}


@frozen_after_init
@dataclass(frozen=True)
class FrozenYamlIndex(Generic[T]):
"""Represents a frozen collection of items that is indexed by the following keys:
Expand All @@ -204,7 +202,7 @@ def __init__(self, other: MutableYamlIndex[T]) -> None:
doc_list[idx] = _YamlDocumentIndexNode(paths=FrozenDict(item_map))

data[file_path] = Collection(doc_list)
self._data = FrozenDict(data)
object.__setattr__(self, "_data", FrozenDict(data))

def transform_values(self, func: Callable[[T], Optional[R]]) -> FrozenYamlIndex[R]:
"""Transforms the values of the given indexed collection into those that are returned from
Expand Down
30 changes: 15 additions & 15 deletions src/python/pants/backend/openapi/util_rules/generator_process.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,14 @@
from pants.jvm.util_rules import rules as jvm_util_rules
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.meta import frozen_after_init


@unique
class OpenAPIGeneratorType(Enum):
JAVA = "java"


@frozen_after_init
@dataclass(unsafe_hash=True)
@dataclass(frozen=True)
class OpenAPIGeneratorProcess:
argv: tuple[str, ...]
generator_type: OpenAPIGeneratorType
Expand Down Expand Up @@ -64,18 +62,20 @@ def __init__(
extra_jvm_options: Iterable[str] | None = None,
cache_scope: ProcessCacheScope | None = None,
):
self.generator_type = generator_type
self.argv = tuple(argv)
self.input_digest = input_digest
self.description = description
self.level = level
self.output_directories = tuple(output_directories or ())
self.output_files = tuple(output_files or ())
self.extra_env = FrozenDict(extra_env or {})
self.extra_classpath_entries = tuple(extra_classpath_entries or ())
self.extra_immutable_input_digests = FrozenDict(extra_immutable_input_digests or {})
self.extra_jvm_options = tuple(extra_jvm_options or ())
self.cache_scope = cache_scope
object.__setattr__(self, "generator_type", generator_type)
object.__setattr__(self, "argv", tuple(argv))
object.__setattr__(self, "input_digest", input_digest)
object.__setattr__(self, "description", description)
object.__setattr__(self, "level", level)
object.__setattr__(self, "output_directories", tuple(output_directories or ()))
object.__setattr__(self, "output_files", tuple(output_files or ()))
object.__setattr__(self, "extra_env", FrozenDict(extra_env or {}))
object.__setattr__(self, "extra_classpath_entries", tuple(extra_classpath_entries or ()))
object.__setattr__(
self, "extra_immutable_input_digests", FrozenDict(extra_immutable_input_digests or {})
)
object.__setattr__(self, "extra_jvm_options", tuple(extra_jvm_options or ()))
object.__setattr__(self, "cache_scope", cache_scope)


_GENERATOR_CLASS_NAME = "org.openapitools.codegen.OpenAPIGenerator"
Expand Down
10 changes: 4 additions & 6 deletions src/python/pants/backend/project_info/dependents.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
from pants.option.option_types import BoolOption
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.meta import frozen_after_init
from pants.util.ordered_set import FrozenOrderedSet


Expand Down Expand Up @@ -44,8 +43,7 @@ async def map_addresses_to_dependents(all_targets: AllUnexpandedTargets) -> Addr
)


@frozen_after_init
@dataclass(unsafe_hash=True)
@dataclass(frozen=True)
class DependentsRequest:
addresses: FrozenOrderedSet[Address]
transitive: bool
Expand All @@ -54,9 +52,9 @@ class DependentsRequest:
def __init__(
self, addresses: Iterable[Address], *, transitive: bool, include_roots: bool
) -> None:
self.addresses = FrozenOrderedSet(addresses)
self.transitive = transitive
self.include_roots = include_roots
object.__setattr__(self, "addresses", FrozenOrderedSet(addresses))
object.__setattr__(self, "transitive", transitive)
object.__setattr__(self, "include_roots", include_roots)


class Dependents(DeduplicatedCollection[Address]):
Expand Down
Loading

0 comments on commit a3d9e25

Please sign in to comment.