Skip to content

Commit a85f303

Browse files
panzhufengcopybara-github
authored andcommitted
Do not align by ids for small list and dict objects, which should be aligned by path by first. Otherwise, the build_diff API would return non-empty results on two identical configs. See the added test as an example.
PiperOrigin-RevId: 557540955
1 parent b1d726d commit a85f303

File tree

2 files changed

+19
-3
lines changed

2 files changed

+19
-3
lines changed

fiddle/_src/diffing.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -481,9 +481,11 @@ def align_heuristically(old: Any, new: Any, old_name='old', new_name='new'):
481481
old_by_id = daglish_legacy.collect_value_by_id(old, memoizable_only=True)
482482
new_by_id = daglish_legacy.collect_value_by_id(new, memoizable_only=True)
483483
for (value_id, value) in old_by_id.items():
484-
if value_id in new_by_id:
485-
if alignment.can_align(value, value):
486-
alignment.align(value, value)
484+
# Do not aligh by id for small container objects.
485+
if not _is_small_container(value):
486+
if value_id in new_by_id:
487+
if alignment.can_align(value, value):
488+
alignment.align(value, value)
487489

488490
# Second pass: align any objects that are reachable by the same path.
489491
path_to_old = daglish_legacy.collect_value_by_path(old, memoizable_only=True)
@@ -504,6 +506,13 @@ def align_heuristically(old: Any, new: Any, old_name='old', new_name='new'):
504506
return alignment
505507

506508

509+
def _is_small_container(value):
510+
"""Returns if value is a small Sequence or Mapping."""
511+
if isinstance(value, (Sequence, Mapping)) and len(repr(value)) < 80:
512+
return True
513+
return False
514+
515+
507516
def _should_align_by_equality(value):
508517
"""Returns true if two equal copies of `value` should be aligned.
509518

fiddle/_src/diffing_test.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -701,6 +701,13 @@ def test_replace_set(self):
701701
expected_changes=(diffing.ModifyValue(
702702
parse_path('[0]'), set([6])),))
703703

704+
def test_align_heuristically_identical_configs_w_different_ids(self):
705+
a = {'k': [1, 2, 3], 'v': None}
706+
b = {'k': [1, 2, 3], 'v': None}
707+
x = fdl.Config(make_pair, a, b)
708+
y = fdl.Config(make_pair, b, a)
709+
self.check_diff(x, y)
710+
704711
def test_root_must_be_aligned(self):
705712
old = fdl.Config(SimpleClass, 1, 2, 3)
706713
new = fdl.Config(SimpleClass, 4, 5, 6)

0 commit comments

Comments
 (0)