Skip to content

Commit 2b0c82a

Browse files
committed
Address Gemini review on phi3 layout-map tests
- Use a `with` block and explicit UTF-8 encoding when reading preset config JSON, avoiding a file-handle leak. - Rephrase comments/skip-messages to describe CI resource limits generically instead of naming a specific dev machine. - Include attention q/k/v/o projection weights in the per-decoder-block memory estimate used to gate the live-preset build guard; the prior formula only counted the embedding table and FFN matrices, which could underestimate memory for width-classes with large hidden dims.
1 parent 2881e55 commit 2b0c82a

1 file changed

Lines changed: 31 additions & 24 deletions

File tree

keras_hub/src/models/phi3/phi3_backbone_test.py

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,9 @@
1717
# `get_file` call to the Tier-2 test body itself (that's what Tier 3,
1818
# `test_layout_map_live_presets` below, is for).
1919
#
20-
# MEMORY NOTE: this local dev machine cannot load full-scale model dims (see
21-
# gemma_backbone_test.py's identically-named constant for the OOM history
22-
# that established this pattern). What actually matters for the
20+
# MEMORY NOTE: full-scale model dims cannot be loaded under CI resource
21+
# limits (see gemma_backbone_test.py's identically-named constant for the
22+
# OOM history that established this pattern). What actually matters for the
2323
# divisibility/sharding properties this tier tests is the RATIO of query
2424
# heads to kv heads and whether hidden/intermediate/vocab divide the mesh's
2525
# model-axis sizes -- not the absolute parameter count. Both registered Phi3
@@ -54,11 +54,11 @@
5454
"intermediate_dim": 512,
5555
}
5656

57-
# Hard-capped mesh-shape list for this shared 37GB dev machine -- see
57+
# Hard-capped mesh-shape list to stay within CI resource limits -- see
5858
# gemma_backbone_test.py's identically-named constant for the full
5959
# rationale and OOM history. Shapes 8x8, 16x16, 4x4x4, 4x4x8 (64-256
60-
# virtual devices) are deliberately dropped; do not attempt them on this
61-
# box.
60+
# virtual devices) are deliberately dropped; do not attempt them in this
61+
# environment.
6262
CAPPED_MESH_SHAPES = [
6363
(2, 4),
6464
(1, 8),
@@ -277,7 +277,7 @@ def test_layout_map_mesh_shapes(self, dims, mesh_shape):
277277
)
278278
init_kwargs = {k: v for k, v in dims.items() if k != "source_preset"}
279279
with distribution.scope():
280-
# bfloat16: a memory mitigation for this shared dev machine --
280+
# bfloat16: a memory mitigation for the local test environment --
281281
# spec assertions are dtype-independent.
282282
model = Phi3Backbone(dtype="bfloat16", **init_kwargs)
283283
_assert_phi3_shardings_and_coverage(self, model, layout_map)
@@ -295,9 +295,9 @@ def test_layout_map_live_presets(self):
295295
# divisibility-relevant dims so width-classes that share a config
296296
# (both registered Phi3 presets are the same "mini" architecture at
297297
# different context lengths) are only built once per mesh shape -- a
298-
# memory/time necessity on this machine, while every preset in the
299-
# registry is still fetched and evaluated, preserving full registry
300-
# coverage.
298+
# memory/time necessity under CI resource limits, while every preset
299+
# in the registry is still fetched and evaluated, preserving full
300+
# registry coverage.
301301
dim_keys = (
302302
"vocabulary_size",
303303
"num_query_heads",
@@ -310,7 +310,8 @@ def test_layout_map_live_presets(self):
310310
for preset in Phi3Backbone.presets:
311311
try:
312312
path = get_file(preset, CONFIG_FILE)
313-
cfg = json.load(open(path))["config"]
313+
with open(path, encoding="utf-8") as f:
314+
cfg = json.load(f)["config"]
314315
except Exception as e:
315316
# A preset this account can't reach (e.g. an unaccepted
316317
# Kaggle license consent click-through) is logged, not
@@ -320,7 +321,7 @@ def test_layout_map_live_presets(self):
320321
# num_layers is forced to 1 below regardless of the real
321322
# value -- layout rules are per-decoder-block regexes, so
322323
# depth is irrelevant to spec matching/divisibility, and 1
323-
# layer keeps build memory bounded on this shared machine.
324+
# layer keeps build memory bounded under CI resource limits.
324325
cfg = dict(cfg)
325326
cfg["num_layers"] = 1
326327
key = tuple(cfg.get(k) for k in dim_keys)
@@ -379,19 +380,25 @@ def test_layout_map_live_presets(self):
379380
skip_reasons.append(reason)
380381
continue
381382

382-
# Memory-budget guard: this shared dev machine cannot
383-
# locally build full-scale presets. Estimate this
383+
# Memory-budget guard: full-scale presets cannot be
384+
# built locally under CI resource limits. Estimate this
384385
# width-class's single-decoder-block bf16 footprint
385-
# (embedding table + one FFN block's 3 matrices, times
386-
# a 3x safety margin for JAX/XLA transient copies
387-
# during construction/resharding) and skip the actual
388-
# build if it exceeds a conservative local threshold.
389-
# The config-fetch, dedup, and divisibility-skip logic
390-
# above still exercises every registry preset either
391-
# way; only the expensive build+assert step is capped.
386+
# (embedding table + attention projections + one FFN
387+
# block's 3 matrices, times a 3x safety margin for
388+
# JAX/XLA transient copies during construction/
389+
# resharding) and skip the actual build if it exceeds a
390+
# conservative local threshold. The config-fetch, dedup,
391+
# and divisibility-skip logic above still exercises
392+
# every registry preset either way; only the expensive
393+
# build+assert step is capped.
392394
est_params = (
393395
cfg["vocabulary_size"] * cfg["hidden_dim"]
394396
+ 3 * cfg["hidden_dim"] * cfg["intermediate_dim"]
397+
# Attention q/k/v/o projections: upper-bounded as
398+
# four hidden_dim x hidden_dim matrices (exact for
399+
# full MHA; GQA configs use less, so this stays a
400+
# conservative overestimate for those).
401+
+ 4 * cfg["hidden_dim"] * cfg["hidden_dim"]
395402
)
396403
est_bytes = est_params * 2 * 3 # bf16 * safety margin
397404
max_local_bytes = 300 * 1024 * 1024 # 300MB
@@ -400,9 +407,9 @@ def test_layout_map_live_presets(self):
400407
f"{combo_label}: estimated build memory "
401408
f"~{est_bytes / 1e9:.2f}GB exceeds the "
402409
f"{max_local_bytes / 1e6:.0f}MB local safety "
403-
"threshold on this shared, RAM-constrained "
404-
"dev machine -- verify this width-class on a "
405-
"machine with more RAM or in CI"
410+
"threshold under CI resource limits -- verify "
411+
"this width-class on a machine with more RAM "
412+
"or in a less-constrained CI environment"
406413
)
407414
skip_reasons.append(reason)
408415
continue

0 commit comments

Comments
 (0)