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

Docker: Support multiple cache_from values for docker_image. #20600

Merged
merged 9 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/docs/docker/index.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,10 @@ docker_image(
"type": "local",
"dest": "/tmp/docker-cache/pants-example"
},
cache_from={
cache_from=[{
"type": "local",
"src": "/tmp/docker-cache/pants-example"
}
}]
)
```

Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/backend/docker/goals/package_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from pants.backend.docker.subsystems.docker_options import DockerOptions
from pants.backend.docker.target_types import (
DockerBuildKitOptionField,
DockerBuildOptionFieldListOfMultiValueDictMixin,
DockerBuildOptionFieldMixin,
DockerBuildOptionFieldMultiValueDictMixin,
DockerBuildOptionFieldMultiValueMixin,
Expand Down Expand Up @@ -336,6 +337,7 @@ def get_build_options(
(
DockerBuildOptionFieldMixin,
DockerBuildOptionFieldMultiValueDictMixin,
DockerBuildOptionFieldListOfMultiValueDictMixin,
DockerBuildOptionFieldValueMixin,
DockerBuildOptionFieldMultiValueMixin,
DockerBuildOptionFlagFieldMixin,
Expand Down
5 changes: 3 additions & 2 deletions src/python/pants/backend/docker/goals/package_image_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,7 @@ def test_docker_cache_from_option(rule_runner: RuleRunner) -> None:
"""\
docker_image(
name="img1",
cache_from={"type": "local", "dest": "/tmp/docker/pants-test-cache"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this old value continue to work? Or does it become a hard error after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be a hard error as it is validating against a list intarget.py below

Copy link
Contributor

@huonw huonw Feb 27, 2024

Choose a reason for hiding this comment

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

Ah I see. That's potentially something we should try to improve, given this is a feature released as stable and even highlighted in release notes (e.g. https://www.pantsbuild.org/blog/2024/01/24/pants-2-19#docker-improvements), so making a change like this would mean that any users who've adopted it will have a harder time upgrading.

Do we have any path for doing this with some sort of a deprecation and guidance for users? I can think of:

  • add this as a field with a new name and deprecate the old one
  • allow it to be either the dict or a list of such (and can emit deprecation warnings about the dict version, to eventually be able to remove that code)
  • anything else?

Copy link
Contributor Author

@riisi riisi Feb 27, 2024

Choose a reason for hiding this comment

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

Yes, I was wondering what we want to do / usually do there.

Probably the easiest is to modify the ListOfDictStringToStringField to accept a single dict - I think that would be reasonable in general.

I'm not sure if we would want a deprecation - or just always allow both a single dict / list of dicts.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, nice catch @huonw

I agree that translating a dict value into a single element list wrapping the dict is a sensible thing to do.

Question is if we want to accept this as a feature long term, or if we want to issue a warning doing so, to urge users to explicitly use a single value list?

I think supporting this as a feature does align with how list options are supported on the cli. I.e. --flag=1 --flag=2 collects into a list option of [1, 2]. It's not the same, for sure, but the semantics are close enough I think.

cache_from=[{"type": "local", "dest": "/tmp/docker/pants-test-cache1"}, {"type": "local", "dest": "/tmp/docker/pants-test-cache2"}],
)
"""
),
Expand All @@ -1164,7 +1164,8 @@ def check_docker_proc(process: Process):
"/dummy/docker",
"buildx",
"build",
"--cache-from=type=local,dest=/tmp/docker/pants-test-cache",
"--cache-from=type=local,dest=/tmp/docker/pants-test-cache1",
"--cache-from=type=local,dest=/tmp/docker/pants-test-cache2",
"--output=type=docker",
"--pull=False",
"--tag",
Expand Down
48 changes: 39 additions & 9 deletions src/python/pants/backend/docker/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
DictStringToStringField,
Field,
InvalidFieldException,
ListOfDictStringToStringField,
OptionalSingleSourceField,
StringField,
StringSequenceField,
Expand Down Expand Up @@ -312,6 +313,23 @@ def options(self, value_formatter: OptionValueFormatter, **kwargs) -> Iterator[s
)


class DockerBuildOptionFieldListOfMultiValueDictMixin(ListOfDictStringToStringField):
"""Inherit this mixin class to provide multiple key-value options to docker build:

`--flag=key1=value1,key2=value2 --flag=key3=value3,key4=value4`
"""

docker_build_option: ClassVar[str]

@final
def options(self, value_formatter: OptionValueFormatter, **kwargs) -> Iterator[str]:
if self.value:
for item in self.value:
yield f"{self.docker_build_option}=" + ",".join(
f"{key}={value_formatter(value)}" for key, value in item.items()
)


class DockerBuildKitOptionField:
"""Mixin to indicate a BuildKit-specific option."""

Expand All @@ -330,6 +348,10 @@ class DockerImageBuildImageCacheToField(
f"""
Export image build cache to an external cache destination.

Note that Docker [supports](https://docs.docker.com/build/cache/backends/#multiple-caches)
multiple cache sources - Pants will pass these as multiple `--cache_from` arguments to the
Docker CLI. Docker will only use the first cache hit (i.e. the image exists) in the build.

{DockerBuildKitOptionField.required_help}

Example:
Expand All @@ -340,10 +362,10 @@ class DockerImageBuildImageCacheToField(
"type": "local",
"dest": "/tmp/docker-cache/example"
}},
cache_from={{
cache_from=[{{
"type": "local",
"src": "/tmp/docker-cache/example"
}}
}}]
)

{_interpolation_help.format(kind="Values")}
Expand All @@ -353,12 +375,14 @@ class DockerImageBuildImageCacheToField(


class DockerImageBuildImageCacheFromField(
DockerBuildOptionFieldMultiValueDictMixin, DictStringToStringField, DockerBuildKitOptionField
DockerBuildOptionFieldListOfMultiValueDictMixin,
ListOfDictStringToStringField,
DockerBuildKitOptionField,
):
alias = "cache_from"
help = help_text(
f"""
Use an external cache source when building the image.
Use external cache sources when building the image.

{DockerBuildKitOptionField.required_help}

Expand All @@ -368,12 +392,18 @@ class DockerImageBuildImageCacheFromField(
name="example-local-cache-backend",
cache_to={{
"type": "local",
"dest": "/tmp/docker-cache/example"
"dest": "/tmp/docker-cache/primary"
}},
cache_from={{
"type": "local",
"src": "/tmp/docker-cache/example"
}}
cache_from=[
{{
"type": "local",
"src": "/tmp/docker-cache/primary"
}},
{{
"type": "local",
"src": "/tmp/docker-cache/secondary"
}}
]
)

{_interpolation_help.format(kind="Values")}
Expand Down
33 changes: 33 additions & 0 deletions src/python/pants/engine/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -1918,6 +1918,39 @@ def compute_value(
return FrozenDict(value_or_default)


class ListOfDictStringToStringField(Field):
value: Optional[Tuple[FrozenDict[str, str]]]
default: ClassVar[Optional[list[FrozenDict[str, str]]]] = None

@classmethod
def compute_value(
cls, raw_value: Optional[list[Dict[str, str]]], address: Address
) -> Optional[Tuple[FrozenDict[str, str], ...]]:
value_or_default = super().compute_value(raw_value, address)
if value_or_default is None:
return None
invalid_type_exception = InvalidFieldTypeException(
address,
cls.alias,
raw_value,
expected_type="a list of dictionaries (or a single dictionary) of string -> string",
)

# Also support passing in a single dictionary by wrapping it
if not isinstance(value_or_default, list):
value_or_default = [value_or_default]

result_lst: list[FrozenDict[str, str]] = []
for item in value_or_default:
if not isinstance(item, collections.abc.Mapping):
raise invalid_type_exception
if not all(isinstance(k, str) and isinstance(v, str) for k, v in item.items()):
raise invalid_type_exception
result_lst.append(FrozenDict(item))

return tuple(result_lst)


class NestedDictStringToStringField(Field):
value: Optional[FrozenDict[str, FrozenDict[str, str]]]
default: ClassVar[Optional[FrozenDict[str, FrozenDict[str, str]]]] = None
Expand Down
29 changes: 29 additions & 0 deletions src/python/pants/engine/target_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
InvalidFieldTypeException,
InvalidGeneratedTargetException,
InvalidTargetException,
ListOfDictStringToStringField,
MultipleSourcesField,
NestedDictStringToStringField,
OptionalSingleSourceField,
Expand Down Expand Up @@ -870,6 +871,34 @@ class ExampleDefault(DictStringToStringField):
assert ExampleDefault(None, addr).value == FrozenDict({"default": "val"})


def test_list_of_dict_string_to_string_field() -> None:
class Example(ListOfDictStringToStringField):
alias = "example"

addr = Address("", target_name="example")

assert Example(None, addr).value is None
assert Example([{}], addr).value == (FrozenDict(),)
assert Example([{"hello": "world"}], addr).value == (FrozenDict({"hello": "world"}),)
# Test support for single dict not passed in a list
assert Example({"hello": "world"}, addr).value == (FrozenDict({"hello": "world"}),)

def assert_invalid_type(raw_value: Any) -> None:
with pytest.raises(InvalidFieldTypeException):
Example(raw_value, addr)

for v in [0, [0], [object()], ["hello"], [["hello"]], [{"hello": 0}], [{0: "world"}]]:
assert_invalid_type(v)

# Test that a default can be set.
class ExampleDefault(ListOfDictStringToStringField):
alias = "example"
# Note that we use `FrozenDict` so that the object can be hashable.
default = [FrozenDict({"default": "val"})]

assert ExampleDefault(None, addr).value == (FrozenDict({"default": "val"}),)


def test_nested_dict_string_to_string_field() -> None:
class Example(NestedDictStringToStringField):
alias = "example"
Expand Down