diff --git a/metaflow/plugins/cards/component_serializer.py b/metaflow/plugins/cards/component_serializer.py index 697934099d5..873c080a10b 100644 --- a/metaflow/plugins/cards/component_serializer.py +++ b/metaflow/plugins/cards/component_serializer.py @@ -545,13 +545,10 @@ def _finalize(self): if len(editable_cards_meta) == 0: return - # Create the `self._card_id_map` lookup table which maps card `id` to `uuid`. - # This table has access to all cards with `id`s set to them. - card_ids = [] - for card_meta in all_card_meta: - if card_meta["card_id"] is not None: - self._card_id_map[card_meta["card_id"]] = card_meta["uuid"] - card_ids.append(card_meta["card_id"]) + # Resolve card IDs before any early returns to avoid duplicate ID issues (#3347). + # This ensures both IndexError from non-editable duplicates and silent content loss + # are prevented. + self._resolve_card_ids(all_card_meta, editable_cards_meta) # If there is only one editable card then this card becomes `self._default_editable_card` if len(editable_cards_meta) == 1: @@ -566,30 +563,6 @@ def _finalize(self): if len(none_id_cards) == 1: self._default_editable_card = none_id_cards[0]["uuid"] - # If the size of the set of ids is not equal to total number of cards with ids then warn the user that we cannot disambiguate - # so `current.card['my_card_id']` won't work. - id_set = set(card_ids) - if len(card_ids) != len(id_set): - non_unique_ids = [ - idx - for idx in id_set - if len(list(filter(lambda x: x["card_id"] == idx, not_none_id_cards))) - > 1 - ] - nui = ", ".join(non_unique_ids) - # throw a warning that decorators have non-unique Ids - self._warning( - ( - "Multiple `@card` decorator have been annotated with duplicate ids : %s. " - "`current.card['%s']` will not work" - ) - % (nui, non_unique_ids[0]) - ) - - # remove the non unique ids from the `self._card_id_map` - for idx in non_unique_ids: - del self._card_id_map[idx] - # if a @card has `customize=True` in the arguments then there should only be one @card with `customize=True`. This @card will be the _default_editable_card customize_cards = [c for c in editable_cards_meta if c["customize"]] if len(customize_cards) > 1: @@ -604,6 +577,49 @@ def _finalize(self): # since `editable_cards_meta` hold only `editable=True` by default we can just set this card here. self._default_editable_card = customize_cards[0]["uuid"] + def _resolve_card_ids(self, all_card_meta, editable_cards_meta): + """ + Resolve card IDs and populate self._card_id_map. + + When duplicate IDs exist: + - If exactly one colliding card is editable, resolve the ID to that card + - Otherwise, remove the ID from the map and warn + + This fixes issue #3347 where duplicate IDs between editable and non-editable + cards caused IndexError or silent content loss. + """ + # Group cards by their ID + cards_by_id = {} + for card_meta in all_card_meta: + card_id = card_meta["card_id"] + if card_id is not None: + if card_id not in cards_by_id: + cards_by_id[card_id] = [] + cards_by_id[card_id].append(card_meta) + + # Resolve each ID + for card_id, cards_with_id in cards_by_id.items(): + if len(cards_with_id) == 1: + # No collision, simple case + self._card_id_map[card_id] = cards_with_id[0]["uuid"] + else: + # Collision: check if exactly one is editable + editable_with_id = [c for c in cards_with_id if c["editable"]] + if len(editable_with_id) == 1: + # Exactly one editable card with this ID: prefer it + self._card_id_map[card_id] = editable_with_id[0]["uuid"] + else: + # Multiple editable cards or no editable cards with this ID: warn and drop + card_types = ", ".join(c["type"] for c in cards_with_id) + self._warning( + ( + "Multiple `@card` decorators have duplicate id '%s' (%s). " + "`current.card['%s']` will not work." + ) + % (card_id, card_types, card_id) + ) + # Do not add this ID to the map + def __getitem__(self, key): """ Choose a specific card for manipulation. diff --git a/test/core/tests/card_duplicate_id_regression.py b/test/core/tests/card_duplicate_id_regression.py new file mode 100644 index 00000000000..66f5f126677 --- /dev/null +++ b/test/core/tests/card_duplicate_id_regression.py @@ -0,0 +1,64 @@ +from metaflow_test import MetaflowTest, ExpectationFailed, steps, tag + + +class CardDuplicateIdRegressionTest(MetaflowTest): + """ + Regression test for issue #3347: Stacked @card with duplicate id from + a non-editable card should not raise IndexError or silently discard content. + + Test scenarios: + 1. Non-editable card with id="mycard" + editable card with id="mycard" + -> Should resolve to the editable card + 2. The flow should complete successfully without IndexError + """ + + PRIORITY = 3 + SKIP_GRAPHS = [ + "simple_switch", + "nested_switch", + "branch_in_switch", + "foreach_in_switch", + "switch_in_branch", + "switch_in_foreach", + "recursive_switch", + "recursive_switch_inside_foreach", + ] + + @tag('card(type="default_json",id="mycard")') # non-editable + @tag('card(type="blank",id="mycard")') # editable, duplicate id + @steps(0, ["start"]) + def step_start(self): + from metaflow import current + from metaflow.plugins.cards.card_modules.basic import MarkdownComponent + + # This should not fail - content should go to the editable blank card + current.card["mycard"].append(MarkdownComponent("# Test Content")) + self.content_added = True + + @steps(0, ["end"], required=True) + def step_end(self): + # Verify we successfully completed + assert self.content_added + self.success = True + + @steps(1, ["all"]) + def step_all(self): + pass + + def check_results(self, flow, checker): + run = checker.get_run() + if run is None: + # CLI check + for step in flow: + if step.name == "end": + # Ensure we reach the end without IndexError + checker.assert_artifact(step.name, "success", True) + elif step.name == "start": + checker.assert_artifact(step.name, "content_added", True) + else: + # Metadata check + for step in flow: + if step.name == "end": + checker.assert_artifact(step.name, "success", True) + elif step.name == "start": + checker.assert_artifact(step.name, "content_added", True) diff --git a/test/unit/test_card_duplicate_id.py b/test/unit/test_card_duplicate_id.py new file mode 100644 index 00000000000..70e27f5b872 --- /dev/null +++ b/test/unit/test_card_duplicate_id.py @@ -0,0 +1,264 @@ +""" +Regression tests for issue #3347: Stacked @card — duplicate id from a non-editable card +raises IndexError, or silently discards card content. +""" + +import pytest +from unittest.mock import Mock, MagicMock + +from metaflow.plugins.cards.component_serializer import CardComponentCollector +from metaflow.plugins.cards.card_modules.basic import MarkdownComponent + + +class MockLogger: + """Mock logger to capture warnings.""" + + def __init__(self): + self.messages = [] + + def __call__(self, msg, timestamp=False, bad=False): + self.messages.append(msg) + + +@pytest.fixture +def logger(): + return MockLogger() + + +@pytest.fixture +def card_creator(): + """Mock card creator.""" + return Mock() + + +@pytest.fixture +def collector(logger, card_creator): + """Create a CardComponentCollector with mocked dependencies.""" + return CardComponentCollector(logger=logger, card_creator=card_creator) + + +def test_duplicate_id_with_non_editable_card_no_longer_raises_index_error( + collector, logger +): + """ + Test case 1 from issue #3347: + When duplicate ids exist across editable and non-editable cards, + _finalize() used to raise IndexError. After fix, it should handle gracefully. + + Flow: + @card(type="default_json", id="mycard") # non-editable + @card(type="blank") # editable, no id + @card(type="blank", id="mycard") # editable, duplicate id + + Expected: Since only ONE editable card has id="mycard", it should win. + """ + # Add three cards: one non-editable with id, one editable without id, one editable with duplicate id + collector._add_card( + card_type="default_json", + card_id="mycard", + decorator_attributes={"type": "default_json"}, + card_options={}, + editable=False, # default_json has ALLOW_USER_COMPONENTS=False + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + collector._add_card( + card_type="blank", + card_id=None, + decorator_attributes={"type": "blank"}, + card_options={}, + editable=True, # blank has ALLOW_USER_COMPONENTS=True + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + editable_card_with_id = collector._add_card( + card_type="blank", + card_id="mycard", # duplicate id + decorator_attributes={"type": "blank"}, + card_options={}, + editable=True, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + + # After the fix, this should not raise IndexError + collector._finalize() + + # Since only one editable card has "mycard", it should win + assert "mycard" in collector._card_id_map + assert collector._card_id_map["mycard"] == editable_card_with_id["uuid"] + + # The editable card should be accessible + card_meta = collector._cards_meta[collector._card_id_map["mycard"]] + assert card_meta["editable"] is True + assert card_meta["type"] == "blank" + + +def test_silent_content_loss_with_single_editable_card(collector, logger): + """ + Test case 2 from issue #3347: + When there's exactly one editable card and a non-editable card shares its id, + content appended to that id may be silently lost. + + Flow: + @card(type="default_json", id="mycard") # non-editable + @card(type="blank", id="mycard") # editable, duplicate id + @step + def start(self): + current.card["mycard"].append(MarkdownComponent("# IMPORTANT")) + """ + # Add two cards with duplicate id: one non-editable, one editable + collector._add_card( + card_type="default_json", + card_id="mycard", + decorator_attributes={"type": "default_json"}, + card_options={}, + editable=False, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + collector._add_card( + card_type="blank", + card_id="mycard", # duplicate id + decorator_attributes={"type": "blank"}, + card_options={}, + editable=True, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + + # Finalize - with only 1 editable card, early return happens before duplicate check + collector._finalize() + + # The _card_id_map should resolve "mycard" to one card, but which one? + # If it resolves to the non-editable default_json, content will be lost + # because non-editable cards don't accept user components + + # Verify that _finalize() returned early (only 1 editable card) + assert collector._default_editable_card is not None + + # Check which card "mycard" resolves to + card_uuid = collector._card_id_map.get("mycard") + assert card_uuid is not None + + # Before the fix, this might resolve to the non-editable card, + # silently discarding content. After the fix, it should either: + # 1. Resolve to the editable card, or + # 2. Be removed from _card_id_map with a warning + + +def test_duplicate_id_across_multiple_non_editable_cards(collector): + """ + Additional test: Multiple non-editable cards with same id. + """ + collector._add_card( + card_type="default_json", + card_id="mycard", + decorator_attributes={"type": "default_json"}, + card_options={}, + editable=False, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + collector._add_card( + card_type="taskspec_card", # another non-editable card type + card_id="mycard", + decorator_attributes={"type": "taskspec_card"}, + card_options={}, + editable=False, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + + # With no editable cards, _finalize should return early without error + # But it should still validate ids if needed + collector._finalize() + + # No default editable card should be set + assert collector._default_editable_card is None + + +def test_multiple_editable_cards_with_same_id_drops_id(collector, logger): + """ + When multiple editable cards have the same ID, the ID should be dropped with a warning. + """ + collector._add_card( + card_type="blank", + card_id="mycard", + decorator_attributes={"type": "blank"}, + card_options={}, + editable=True, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + collector._add_card( + card_type="default", # also editable + card_id="mycard", + decorator_attributes={"type": "default"}, + card_options={}, + editable=True, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + + collector._finalize() + + # Multiple editable cards with same ID: should be dropped + assert "mycard" not in collector._card_id_map + + # A warning should have been logged + assert any("duplicate id" in msg.lower() for msg in logger.messages) + + +def test_unique_ids_work_correctly(collector): + """ + Sanity check: unique ids should work fine. + """ + collector._add_card( + card_type="default_json", + card_id="card1", + decorator_attributes={"type": "default_json"}, + card_options={}, + editable=False, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + collector._add_card( + card_type="blank", + card_id="card2", + decorator_attributes={"type": "blank"}, + card_options={}, + editable=True, + customize=False, + suppress_warnings=False, + runtime_card=False, + refresh_interval=5, + ) + + collector._finalize() + + # Both ids should be in the map + assert "card1" in collector._card_id_map + assert "card2" in collector._card_id_map + # Default editable card should be set (only 1 editable card) + assert collector._default_editable_card is not None