Skip to content

Commit 404c2b3

Browse files
authored
fix: Support for docker compose > v2 (5739) (#5740)
* fix: Support for docker compose > v2 (5739) * Adding integ tests to verify docker compose version * fix: address review comments (iteration #1) * fix: address review comments (iteration #3)
1 parent 5235dd6 commit 404c2b3

File tree

6 files changed

+391
-17
lines changed

6 files changed

+391
-17
lines changed

sagemaker-core/src/sagemaker/core/local/image.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ def __init__(
138138
def _get_compose_cmd_prefix():
139139
"""Gets the Docker Compose command.
140140
141-
The method initially looks for 'docker compose' v2
141+
The method initially looks for 'docker compose' v2+
142142
executable, if not found looks for 'docker-compose' executable.
143143
144144
Returns:
@@ -162,10 +162,12 @@ def _get_compose_cmd_prefix():
162162
"Proceeding to check for 'docker-compose' CLI."
163163
)
164164

165-
if output and "v2" in output.strip():
166-
logger.info("'Docker Compose' found using Docker CLI.")
167-
compose_cmd_prefix.extend(["docker", "compose"])
168-
return compose_cmd_prefix
165+
if output:
166+
match = re.search(r"v(\d+)", output.strip())
167+
if match and int(match.group(1)) >= 2:
168+
logger.info("'Docker Compose' found using Docker CLI.")
169+
compose_cmd_prefix.extend(["docker", "compose"])
170+
return compose_cmd_prefix
169171

170172
if shutil.which("docker-compose") is not None:
171173
logger.info("'Docker Compose' found using Docker Compose CLI.")

sagemaker-core/src/sagemaker/core/modules/local_core/local_container.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -593,7 +593,7 @@ def _get_data_source_local_path(self, data_source: DataSource):
593593
def _get_compose_cmd_prefix(self) -> List[str]:
594594
"""Gets the Docker Compose command.
595595
596-
The method initially looks for 'docker compose' v2
596+
The method initially looks for 'docker compose' v2+
597597
executable, if not found looks for 'docker-compose' executable.
598598
599599
Returns:
@@ -617,10 +617,12 @@ def _get_compose_cmd_prefix(self) -> List[str]:
617617
"Proceeding to check for 'docker-compose' CLI."
618618
)
619619

620-
if output and "v2" in output.strip():
621-
logger.info("'Docker Compose' found using Docker CLI.")
622-
compose_cmd_prefix.extend(["docker", "compose"])
623-
return compose_cmd_prefix
620+
if output:
621+
match = re.search(r"v(\d+)", output.strip())
622+
if match and int(match.group(1)) >= 2:
623+
logger.info("'Docker Compose' found using Docker CLI.")
624+
compose_cmd_prefix.extend(["docker", "compose"])
625+
return compose_cmd_prefix
624626

625627
if shutil.which("docker-compose") is not None:
626628
logger.info("'Docker Compose' found using Docker Compose CLI.")

sagemaker-core/tests/unit/modules/local_core/test_local_container.py

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,82 @@ def test_get_compose_cmd_prefix_not_found(
638638
with pytest.raises(ImportError, match="Docker Compose is not installed"):
639639
container._get_compose_cmd_prefix()
640640

641+
@patch("sagemaker.core.modules.local_core.local_container.subprocess.check_output")
642+
def test_get_compose_cmd_prefix_docker_compose_v5(
643+
self, mock_check_output, mock_session, basic_channel
644+
):
645+
"""Test _get_compose_cmd_prefix accepts Docker Compose v5"""
646+
container = _LocalContainer(
647+
training_job_name="test-job",
648+
instance_type="local",
649+
instance_count=1,
650+
image="test-image:latest",
651+
container_root="/tmp/test",
652+
input_data_config=[basic_channel],
653+
environment={},
654+
hyper_parameters={},
655+
container_entrypoint=[],
656+
container_arguments=[],
657+
sagemaker_session=mock_session,
658+
)
659+
660+
mock_check_output.return_value = "Docker Compose version v5.1.1"
661+
662+
result = container._get_compose_cmd_prefix()
663+
664+
assert result == ["docker", "compose"]
665+
666+
@patch("sagemaker.core.modules.local_core.local_container.subprocess.check_output")
667+
def test_get_compose_cmd_prefix_docker_compose_v3(
668+
self, mock_check_output, mock_session, basic_channel
669+
):
670+
"""Test _get_compose_cmd_prefix accepts Docker Compose v3"""
671+
container = _LocalContainer(
672+
training_job_name="test-job",
673+
instance_type="local",
674+
instance_count=1,
675+
image="test-image:latest",
676+
container_root="/tmp/test",
677+
input_data_config=[basic_channel],
678+
environment={},
679+
hyper_parameters={},
680+
container_entrypoint=[],
681+
container_arguments=[],
682+
sagemaker_session=mock_session,
683+
)
684+
685+
mock_check_output.return_value = "Docker Compose version v3.0.0"
686+
687+
result = container._get_compose_cmd_prefix()
688+
689+
assert result == ["docker", "compose"]
690+
691+
@patch("sagemaker.core.modules.local_core.local_container.subprocess.check_output")
692+
@patch("sagemaker.core.modules.local_core.local_container.shutil.which")
693+
def test_get_compose_cmd_prefix_docker_compose_v1_rejected(
694+
self, mock_which, mock_check_output, mock_session, basic_channel
695+
):
696+
"""Test _get_compose_cmd_prefix rejects Docker Compose v1"""
697+
container = _LocalContainer(
698+
training_job_name="test-job",
699+
instance_type="local",
700+
instance_count=1,
701+
image="test-image:latest",
702+
container_root="/tmp/test",
703+
input_data_config=[basic_channel],
704+
environment={},
705+
hyper_parameters={},
706+
container_entrypoint=[],
707+
container_arguments=[],
708+
sagemaker_session=mock_session,
709+
)
710+
711+
mock_check_output.return_value = "docker-compose version v1.29.2"
712+
mock_which.return_value = None
713+
714+
with pytest.raises(ImportError, match="Docker Compose is not installed"):
715+
container._get_compose_cmd_prefix()
716+
641717
def test_init_with_container_entrypoint(self, mock_session, basic_channel):
642718
"""Test initialization with container entrypoint"""
643719
container = _LocalContainer(

sagemaker-train/src/sagemaker/train/local/local_container.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -601,7 +601,7 @@ def _get_data_source_local_path(self, data_source: DataSource):
601601
def _get_compose_cmd_prefix(self) -> List[str]:
602602
"""Gets the Docker Compose command.
603603
604-
The method initially looks for 'docker compose' v2
604+
The method initially looks for 'docker compose' v2+
605605
executable, if not found looks for 'docker-compose' executable.
606606
607607
Returns:
@@ -625,10 +625,12 @@ def _get_compose_cmd_prefix(self) -> List[str]:
625625
"Proceeding to check for 'docker-compose' CLI."
626626
)
627627

628-
if output and "v2" in output.strip():
629-
logger.info("'Docker Compose' found using Docker CLI.")
630-
compose_cmd_prefix.extend(["docker", "compose"])
631-
return compose_cmd_prefix
628+
if output:
629+
match = re.search(r"v(\d+)", output.strip())
630+
if match and int(match.group(1)) >= 2:
631+
logger.info("'Docker Compose' found using Docker CLI.")
632+
compose_cmd_prefix.extend(["docker", "compose"])
633+
return compose_cmd_prefix
632634

633635
if shutil.which("docker-compose") is not None:
634636
logger.info("'Docker Compose' found using Docker Compose CLI.")
Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You
4+
# may not use this file except in compliance with the License. A copy of
5+
# the License is located at
6+
#
7+
# http://aws.amazon.com/apache2.0/
8+
#
9+
# or in the "license" file accompanying this file. This file is
10+
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
11+
# ANY KIND, either express or implied. See the License for the specific
12+
# language governing permissions and limitations under the License.
13+
"""Integration tests for Docker Compose version detection fix (issue #5739).
14+
15+
These tests verify that _get_compose_cmd_prefix correctly accepts Docker Compose
16+
versions >= 2 (including v3, v4, v5, etc.) rather than only accepting v2.
17+
18+
The tests run against the real Docker Compose installation on the machine — no mocking.
19+
Requires: Docker with Compose plugin installed (any version >= 2).
20+
"""
21+
from __future__ import absolute_import
22+
23+
import re
24+
import subprocess
25+
import tempfile
26+
27+
import pytest
28+
29+
from sagemaker.core.local.image import _SageMakerContainer
30+
from sagemaker.core.modules.local_core.local_container import (
31+
_LocalContainer as CoreModulesLocalContainer,
32+
)
33+
from sagemaker.core.shapes import Channel, DataSource, S3DataSource
34+
from sagemaker.train.local.local_container import (
35+
_LocalContainer as TrainLocalContainer,
36+
)
37+
38+
39+
def _get_installed_compose_major_version():
40+
"""Return the major version int of the installed Docker Compose, or None."""
41+
try:
42+
output = subprocess.check_output(
43+
["docker", "compose", "version"],
44+
stderr=subprocess.DEVNULL,
45+
encoding="UTF-8",
46+
)
47+
match = re.search(r"v(\d+)", output.strip())
48+
if match:
49+
return int(match.group(1))
50+
except (subprocess.CalledProcessError, FileNotFoundError):
51+
pass
52+
return None
53+
54+
55+
# Skip the entire module if Docker Compose >= 2 is not available
56+
_compose_major = _get_installed_compose_major_version()
57+
pytestmark = pytest.mark.skipif(
58+
_compose_major is None or _compose_major < 2,
59+
reason=f"Docker Compose >= 2 required (found: v{_compose_major})",
60+
)
61+
62+
63+
def _make_basic_channel():
64+
"""Create a minimal Channel for constructing _LocalContainer instances."""
65+
data_source = DataSource(
66+
s3_data_source=S3DataSource(
67+
s3_uri="s3://bucket/data",
68+
s3_data_type="S3Prefix",
69+
s3_data_distribution_type="FullyReplicated",
70+
)
71+
)
72+
return Channel(channel_name="training", data_source=data_source)
73+
74+
75+
def _make_local_container(container_cls):
76+
"""Construct a _LocalContainer with minimal valid args.
77+
78+
sagemaker_session is None since _get_compose_cmd_prefix doesn't use it,
79+
and the Pydantic model rejects Mock objects.
80+
"""
81+
container_root = tempfile.mkdtemp(prefix="sagemaker-integ-compose-")
82+
return container_cls(
83+
training_job_name="integ-test-compose-detection",
84+
instance_type="local",
85+
instance_count=1,
86+
image="test-image:latest",
87+
container_root=container_root,
88+
input_data_config=[_make_basic_channel()],
89+
environment={},
90+
hyper_parameters={},
91+
container_entrypoint=[],
92+
container_arguments=[],
93+
sagemaker_session=None,
94+
)
95+
96+
97+
@pytest.fixture
98+
def _core_modules_container():
99+
return _make_local_container(CoreModulesLocalContainer)
100+
101+
102+
@pytest.fixture
103+
def _train_container():
104+
return _make_local_container(TrainLocalContainer)
105+
106+
107+
class TestDockerComposeVersionDetection:
108+
"""Integration tests for _get_compose_cmd_prefix across all three code locations.
109+
110+
Validates the fix for https://github.com/aws/sagemaker-python-sdk/issues/5739
111+
where Docker Compose v3+ was incorrectly rejected.
112+
"""
113+
114+
def test_sagemaker_core_image_accepts_installed_compose(self):
115+
"""sagemaker-core local/image.py _SageMakerContainer._get_compose_cmd_prefix
116+
should accept the installed Docker Compose version."""
117+
result = _SageMakerContainer._get_compose_cmd_prefix()
118+
119+
assert result == ["docker", "compose"], (
120+
f"Expected ['docker', 'compose'] but got {result}. "
121+
f"Installed Docker Compose is v{_compose_major}."
122+
)
123+
124+
def test_sagemaker_core_modules_local_container_accepts_installed_compose(
125+
self, _core_modules_container
126+
):
127+
"""sagemaker-core modules/local_core/local_container.py
128+
_LocalContainer._get_compose_cmd_prefix should accept the installed version."""
129+
result = _core_modules_container._get_compose_cmd_prefix()
130+
131+
assert result == ["docker", "compose"], (
132+
f"Expected ['docker', 'compose'] but got {result}. "
133+
f"Installed Docker Compose is v{_compose_major}."
134+
)
135+
136+
def test_sagemaker_train_local_container_accepts_installed_compose(
137+
self, _train_container
138+
):
139+
"""sagemaker-train local/local_container.py
140+
_LocalContainer._get_compose_cmd_prefix should accept the installed version."""
141+
result = _train_container._get_compose_cmd_prefix()
142+
143+
assert result == ["docker", "compose"], (
144+
f"Expected ['docker', 'compose'] but got {result}. "
145+
f"Installed Docker Compose is v{_compose_major}."
146+
)
147+
148+
def test_returned_command_is_functional(self):
149+
"""The command returned by _get_compose_cmd_prefix should actually work."""
150+
cmd = _SageMakerContainer._get_compose_cmd_prefix()
151+
152+
# Run the returned command with "version" to prove it's functional
153+
result = subprocess.run(
154+
cmd + ["version"],
155+
capture_output=True,
156+
text=True,
157+
timeout=10,
158+
)
159+
assert result.returncode == 0, (
160+
f"Command {cmd + ['version']} failed: {result.stderr}"
161+
)
162+
assert "version" in result.stdout.lower(), (
163+
f"Unexpected output from {cmd + ['version']}: {result.stdout}"
164+
)
165+
166+
@pytest.mark.skipif(
167+
_compose_major is not None and _compose_major < 3,
168+
reason="This test specifically validates v3+ acceptance (installed is v2)",
169+
)
170+
def test_v3_plus_specifically_accepted(self):
171+
"""When Docker Compose v3+ is installed, it must be accepted — not rejected.
172+
173+
This is the core regression test for issue #5739.
174+
"""
175+
result = _SageMakerContainer._get_compose_cmd_prefix()
176+
assert result == ["docker", "compose"], (
177+
f"Docker Compose v{_compose_major} was rejected. "
178+
"This is the exact bug described in issue #5739."
179+
)

0 commit comments

Comments
 (0)