fix(cards): resolve duplicate stacked @card IDs without IndexError - #3374
fix(cards): resolve duplicate stacked @card IDs without IndexError#3374Shriprasad-P wants to merge 2 commits into
Conversation
…IndexError and content loss The _finalize() method in CardComponentCollector was building the id->uuid lookup over all cards but validating duplicates over editable cards only, and after an early return. This caused two bugs: 1. IndexError when duplicate IDs existed between non-editable and editable cards (e.g., @card(type='default_json', id='x') + @card(type='blank', id='x')) The duplicate was detected but the offender list was empty, causing non_unique_ids[0] to raise IndexError. 2. Silent content loss when exactly one editable card existed and shared an ID with a non-editable card. The early return at len(editable_cards_meta)==1 skipped duplicate checking, and if _card_id_map['x'] resolved to the non-editable card, content appended to current.card['x'] was discarded. Fix: Extract ID resolution into _resolve_card_ids() and call it before any early returns. When duplicate IDs exist: - If exactly one colliding card is editable, resolve to it (editable-wins) - Otherwise, drop the ID from the map with a warning This ensures IDs are resolved over one consistent population and both failure modes are prevented. Adds comprehensive unit tests covering both bug scenarios plus edge cases, and an integration test verifying end-to-end behavior.
Greptile SummaryThis PR centralizes card-ID resolution during collector finalization and introduces an editable-wins policy for duplicate IDs, preventing the reported
Confidence Score: 4/5The implementation appears safe to merge, with only a non-blocking concern about one ineffective unit test. The duplicate-ID resolver implements the stated editable-wins behavior and the meaningful unit and integration scenarios cover the reported failure; the remaining concern is limited to a new test that does not exercise the behavior its name describes. Files Needing Attention: test/unit/test_card_duplicate_id.py
|
| Filename | Overview |
|---|---|
| metaflow/plugins/cards/component_serializer.py | Adds centralized duplicate-ID resolution with an editable-wins policy and warnings for unresolved collisions. |
| test/unit/test_card_duplicate_id.py | Adds focused resolver tests, though the all-non-editable duplicate test does not reach or validate duplicate-ID resolution. |
| test/core/tests/card_duplicate_id_regression.py | Adds an end-to-end stacked-card regression scenario confirming that duplicate editable/non-editable IDs no longer abort flow execution. |
Reviews (1): Last reviewed commit: "Apply pre-commit formatting fixes (black..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| # No default editable card should be set | ||
| assert collector._default_editable_card is None |
There was a problem hiding this comment.
This test never exercises the new resolver. When there are no editable cards, _finalize() returns before _resolve_card_ids() runs, and the only assertion checks that the default card remains unset. The test therefore passes regardless of how duplicate non-editable IDs are handled, giving misleading regression coverage. Assert the intended warning and ID-map behavior after making that path reachable, or remove this test.
1 participant
Fixes
Fixes #3347
Summary
Resolve duplicate stacked
@cardIDs before finalization with an editable-wins policy, so tasks no longer hitIndexErrorand editable card content is not silently discarded.Test plan