Skip to content

Commit 6f7b460

Browse files
author
Bhanu Teja Goshikonda
committed
fix(tensorflow): address review HIGH findings (entrypoint exec, FRAMEWORK default, test correctness)
- Dockerfile.{cpu,cuda}: change ARG FRAMEWORK default from 'tensorflow_runtime' to 'tensorflow' so local builds (which don't override via build-arg) produce images whose telemetry argparse choices are satisfied. Build-image action still overrides from metadata.framework, so production behavior unchanged. - scripts/docker/tensorflow/entrypoint.sh: replace 'eval "$@"' with 'exec "$@"' to match PyTorch's entrypoint. Gives the user command proper PID-1 semantics for signal forwarding (SIGTERM on training-job stop) and avoids shell re-parsing of args. - test/tensorflow/unit/test_smoke.py: replace tf.ones + reduce_min>0 value assertion with tf.random.normal + shape assertion (matches PT's test_gpu_tensor_matmul). The prior assertion tested arithmetic on a specific input, not image health. - test/tensorflow/single_gpu/test_gpu_memory.py: rewrite as 'pool reuse' assertion. The prior test compared 'current' memory before/after with a 50 MB tolerance, which is fragile under TF's BFC allocator (retains pool memory by design). TF has no torch.cuda.empty_cache() analog. New test asserts peak memory does not grow on a second identical allocation cycle. - test/tensorflow/integration/sagemaker/test_experiments_cpu.py: narrow the two 'except Exception: pass' cleanup blocks to 'except ClientError', let ResourceNotFound/ValidationException through silently, log other failures via LOG.warning so silent state leaks don't accumulate in the CI account.
1 parent 785ec1a commit 6f7b460

6 files changed

Lines changed: 55 additions & 40 deletions

File tree

docker/tensorflow/2.21/Dockerfile.cpu

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ RUN CURL_DIR="$(python -c 'import tensorflow as tf, os; print(os.path.join(tf.sy
170170
# Telemetry
171171
COPY scripts/docker/telemetry/deep_learning_container.py /usr/local/bin/deep_learning_container.py
172172
COPY scripts/docker/telemetry/bash_telemetry.sh.template /tmp/bash_telemetry.sh.template
173-
ARG FRAMEWORK="tensorflow_runtime"
173+
ARG FRAMEWORK="tensorflow"
174174
ARG CONTAINER_TYPE="training"
175175
RUN chmod +x /usr/local/bin/deep_learning_container.py \
176176
&& sed -e "s/{{FRAMEWORK}}/${FRAMEWORK}/g" \

docker/tensorflow/2.21/Dockerfile.cuda

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ RUN CURL_DIR="$(python -c 'import tensorflow as tf, os; print(os.path.join(tf.sy
227227
# Telemetry
228228
COPY scripts/docker/telemetry/deep_learning_container.py /usr/local/bin/deep_learning_container.py
229229
COPY scripts/docker/telemetry/bash_telemetry.sh.template /tmp/bash_telemetry.sh.template
230-
ARG FRAMEWORK="tensorflow_runtime"
230+
ARG FRAMEWORK="tensorflow"
231231
ARG CONTAINER_TYPE="training"
232232
RUN chmod +x /usr/local/bin/deep_learning_container.py \
233233
&& sed -e "s/{{FRAMEWORK}}/${FRAMEWORK}/g" \

scripts/docker/tensorflow/entrypoint.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,4 +13,4 @@ fi
1313
# CUDA forward-compatibility (safe no-op on CPU)
1414
bash /usr/local/bin/start_cuda_compat.sh || true
1515

16-
eval "$@"
16+
exec "$@"

test/tensorflow/integration/sagemaker/test_experiments_cpu.py

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,13 @@
1212
"""
1313

1414
import datetime
15+
import logging
1516
import os
1617
import random
1718
import time
1819

1920
import boto3
21+
from botocore.exceptions import ClientError
2022
from sagemaker.core.experiments.experiment import Experiment
2123
from sagemaker.core.experiments.trial import _Trial
2224
from sagemaker.core.experiments.trial_component import _TrialComponent
@@ -25,6 +27,8 @@
2527
from sagemaker.train import ModelTrainer
2628
from test_utils import random_suffix_name
2729

30+
LOG = logging.getLogger(__name__)
31+
2832
RESOURCE_DIR = os.path.join(os.path.dirname(__file__), "resources")
2933
SOURCE_DIR = os.path.join(RESOURCE_DIR, "scripts")
3034
INSTANCE_TYPE = "ml.c5.xlarge"
@@ -107,13 +111,18 @@ def test_experiments_cpu():
107111
trial.remove_trial_component(trial_component_summary.trial_component_name)
108112
trial_component.delete(force_disassociate=True)
109113
finally:
110-
# Best-effort cleanup — keep the test from leaking state on failure.
111-
try:
112-
trial.delete()
113-
except Exception: # noqa: BLE001
114-
pass
114+
# Best-effort cleanup — narrow to AWS SDK errors, log non-not-found
115+
# failures so silent leaks don't accumulate in the CI account.
116+
_safe_delete(trial.delete, f"trial {trial_name}")
115117
time.sleep(1.2) # avoid Experiments control-plane throttling
116-
try:
117-
experiment.delete()
118-
except Exception: # noqa: BLE001
119-
pass
118+
_safe_delete(experiment.delete, f"experiment {experiment_name}")
119+
120+
121+
def _safe_delete(delete_fn, label: str) -> None:
122+
try:
123+
delete_fn()
124+
except ClientError as e:
125+
code = e.response.get("Error", {}).get("Code", "")
126+
if code in ("ResourceNotFound", "ValidationException"):
127+
return
128+
LOG.warning("cleanup failed for %s: %s", label, e)
Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,15 @@
1-
"""GPU memory-allocator smoke test — allocate, free, verify no large leak."""
1+
"""GPU memory-allocator smoke test.
2+
3+
TF's BFC allocator deliberately retains pool memory across `clear_session()` /
4+
`gc.collect()` — `get_memory_info(device)["current"]` reflects allocator-held
5+
bytes, not user-held bytes. Unlike PyTorch's `torch.cuda.empty_cache()`, TF
6+
exposes no public API that forces the pool back to the OS at runtime, so a
7+
"memory after free == memory before alloc" assertion is unreliable.
8+
9+
Instead, assert the property we actually care about: when a tensor is freed,
10+
the next allocation of the same size REUSES the existing pool rather than
11+
growing it. Peak memory across two alloc/free cycles should be flat.
12+
"""
213

314
import gc
415

@@ -7,31 +18,29 @@
718

819
pytestmark = pytest.mark.skipif(not tf.config.list_physical_devices("GPU"), reason="GPU only")
920

10-
# 50 MB tolerance for runtime overhead; TF caches GPU memory so an exact match isn't expected.
11-
LEAK_TOLERANCE_BYTES = 50 * 1024 * 1024
12-
21+
# 5% growth tolerance between cycles for minor allocator bookkeeping.
22+
PEAK_GROWTH_TOLERANCE = 1.05
1323

14-
def test_gpu_memory_returns_to_baseline():
15-
device = "GPU:0"
1624

17-
# Warm up so steady-state allocator overhead is included in the baseline.
18-
warmup = tf.zeros([1024, 1024])
19-
del warmup
25+
def _peak_for_cycle(device: str, shape) -> int:
26+
"""Reset peak stats, alloc + free, return the cycle's peak in bytes."""
27+
tf.config.experimental.reset_memory_stats(device)
28+
t = tf.zeros(shape)
29+
peak = tf.config.experimental.get_memory_info(device)["peak"]
30+
del t
2031
gc.collect()
32+
return peak
2133

22-
baseline = tf.config.experimental.get_memory_info(device)["current"]
2334

24-
# Allocate ~400 MB: 1000 * 1000 * 100 float32 elements.
25-
big = tf.zeros([1000, 1000, 100])
26-
peak = tf.config.experimental.get_memory_info(device)["current"]
27-
assert peak > baseline, f"allocation did not raise current memory: {baseline} -> {peak}"
35+
def test_gpu_memory_reuses_pool_across_cycles():
36+
device = "GPU:0"
37+
shape = [1000, 1000, 100] # ~400 MB float32
2838

29-
del big
30-
tf.keras.backend.clear_session()
31-
gc.collect()
39+
peak1 = _peak_for_cycle(device, shape)
40+
assert peak1 > 0, "allocation did not register in peak memory"
3241

33-
after = tf.config.experimental.get_memory_info(device)["current"]
34-
leaked = after - baseline
35-
assert leaked <= LEAK_TOLERANCE_BYTES, (
36-
f"memory not released: baseline={baseline}, after={after}, leaked={leaked} bytes"
42+
peak2 = _peak_for_cycle(device, shape)
43+
assert peak2 <= peak1 * PEAK_GROWTH_TOLERANCE, (
44+
f"BFC pool grew on identical re-allocation: peak1={peak1}, peak2={peak2} "
45+
f"(tolerance {PEAK_GROWTH_TOLERANCE}x). Suggests freed memory was not reused."
3746
)

test/tensorflow/unit/test_smoke.py

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,13 @@
2121

2222
def test_tensorflow_matmul_runs():
2323
"""Run a tiny matmul on whatever device TF picks (CPU on no-GPU runner).
24-
Asserts the op actually executes and the shape is right.
25-
Master TF analog: testTensorflow2Standalone."""
24+
Asserts the op actually executes and the shape is right."""
2625
import tensorflow as tf
2726

28-
a = tf.constant([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0]]) # (2, 3)
29-
b = tf.constant([[7.0, 8.0], [9.0, 10.0], [11.0, 12.0]]) # (3, 2)
27+
a = tf.random.normal([256, 256])
28+
b = tf.random.normal([256, 256])
3029
c = tf.linalg.matmul(a, b)
31-
assert tuple(c.shape) == (2, 2)
32-
# All four entries are positive — a passing matmul yields non-zero output.
33-
assert tf.reduce_min(c).numpy() > 0
30+
assert tuple(c.shape) == (256, 256)
3431

3532

3633
def test_cpu_devices_detected():

0 commit comments

Comments
 (0)