Skip to content

Stacked @card: duplicate id from a non-editable card raises IndexError, or silently discards card content #3347

Description

@nileshpatil6

Summary

CardComponentCollector._finalize() builds the id -> uuid lookup over all @card decorators, but validates and de-duplicates ids over editable cards only, and does that validation after an early return. Two things follow from that mismatch: a task-killing IndexError, and silent loss of card content.

Both are reachable from ordinary stacked @card decorators. The code is in metaflow/plugins/cards/component_serializer.py, unchanged since #893 (Jan 2022).

1. IndexError kills the step

@card(type="default_json", id="mycard")
@card(type="blank")
@card(type="blank", id="mycard")
@step
def start(self):
    ...
File ".../metaflow/plugins/cards/card_decorator.py", line 312, in task_pre_step
    current.card._finalize()
File ".../metaflow/plugins/cards/component_serializer.py", line 586, in _finalize
    % (nui, non_unique_ids[0])
IndexError: list index out of range
Task failed.

Why: card_ids is populated from all_card_meta, so the duplicate-detection trigger

id_set = set(card_ids)
if len(card_ids) != len(id_set):

fires on a collision contributed by any card. But the offender list is computed over editable cards only:

not_none_id_cards = [c for c in editable_cards_meta if c["card_id"] is not None]
...
non_unique_ids = [idx for idx in id_set
                  if len(list(filter(lambda x: x["card_id"] == idx, not_none_id_cards))) > 1]

default_json has ALLOW_USER_COMPONENTS = False, so it is not in editable_cards_meta. The duplicate is real, the offender list is empty, and non_unique_ids[0] raises. The same applies to the other shipped non-editable types (error, taskspec_card) and to any third-party card type.

2. Silent loss of content when there is exactly one editable card

@card(type="default_json", id="mycard")
@card(type="blank", id="mycard")
@step
def start(self):
    current.card["mycard"].append(Markdown("# IMPORTANT"))

Here len(editable_cards_meta) == 1, so

if len(editable_cards_meta) == 1:
    self._default_editable_card = editable_cards_meta[0]["uuid"]
    return

returns before any duplicate check runs. _card_id_map["mycard"] keeps whichever card registered last. If that is the non-editable default_json, everything appended to current.card["mycard"] is discarded. No warning, task succeeds, content gone.

Suggested direction

Resolve ids over one consistent population, before both early returns, rather than shrinking the map (the class docstring documents current.card['myid'] as working for non-default-editable cards, so the map should keep spanning all cards).

The part I would want your opinion on is the collision policy. When an id is claimed by several cards and exactly one of them is editable, the options are:

  • resolve the id to the editable card, since it is the only colliding card that can render user components; or
  • drop the id and warn, treating any duplicate uniformly.

The first fixes the silent-loss case; the second is simpler and more predictable, but turns something that appears to work today into a warning.

I have a patch implementing the first option, with unit tests covering both failure modes and end-to-end runs of the two flows above. Happy to open a PR if a maintainer confirms the direction, or to switch it to the second option.

Environment

main at d2dd7f9, Python 3.14, Linux.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions