Fix local card storage root resolution

metaflow/plugins/cards/card_cli.py

@@ -349,7 +349,12 @@ def card(ctx):
     )
     metadata(setting_metadata)
     # set the card root to the datastore according to the configuration.
-    root_pth = CardDatastore.get_storage_root(ctx.obj.flow_datastore._storage_impl.TYPE)
+    # Pass the already-resolved artifact datastore root so local/spin cards stay
+    # under that root when no explicit CARD_LOCALROOT is configured.
+    root_pth = CardDatastore.get_storage_root(
+        ctx.obj.flow_datastore._storage_impl.TYPE,
+        datastore_root=ctx.obj.flow_datastore.datastore_root,
+    )
     if root_pth is not None:
         ctx.obj.flow_datastore._storage_impl.datastore_root = root_pth
 

metaflow/plugins/cards/card_client.py

@@ -299,7 +299,11 @@ def _get_flow_datastore(task):
         raise UnresolvableDatastoreException(task)
 
     ds_root = meta_dict.get("ds-root", None)
-    if ds_root:
+    if ds_type == "local" or ds_type == "spin":
+        # Local/spin card roots must follow the same precedence as writers:
+        # explicit CARD_LOCALROOT, then <ds-root>/mf.cards, then legacy lookup.
+        ds_root = CardDatastore.get_storage_root(ds_type, datastore_root=ds_root)
+    elif ds_root:
         ds_root = os.path.join(ds_root, CARD_SUFFIX)
     else:
         ds_root = CardDatastore.get_storage_root(ds_type)

metaflow/plugins/cards/card_datastore.py

@@ -52,35 +52,41 @@ def is_file_present(path):
 
 class CardDatastore(object):
     @classmethod
-    def get_storage_root(cls, storage_type):
+    def get_storage_root(cls, storage_type, datastore_root=None):
         if storage_type == "s3":
             return CARD_S3ROOT
         elif storage_type == "azure":
             return CARD_AZUREROOT
         elif storage_type == "gs":
             return CARD_GSROOT
         elif storage_type == "local" or storage_type == "spin":
-            # Borrowing some of the logic from LocalStorage.get_storage_root
-            result = CARD_LOCALROOT
+            # Prefer an explicit card root when configured. Otherwise keep cards
+            # under the already-resolved artifact datastore root when available.
+            # Fall back to the legacy nearest-parent `.metaflow` / `.metaflow_spin`
+            # lookup only when neither root is known.
+            if CARD_LOCALROOT is not None:
+                return CARD_LOCALROOT
+            if datastore_root:
+                return os.path.join(datastore_root, CARD_SUFFIX)
+
             local_dir = (
                 DATASTORE_SPIN_LOCAL_DIR
                 if storage_type == "spin"
                 else DATASTORE_LOCAL_DIR
             )
-            if result is None:
-                current_path = os.getcwd()
+            current_path = os.getcwd()
+            check_dir = os.path.join(current_path, local_dir)
+            check_dir = os.path.realpath(check_dir)
+            orig_path = check_dir
+            while not os.path.isdir(check_dir):
+                new_path = os.path.dirname(current_path)
+                if new_path == current_path:
+                    # No longer making upward progress so we
+                    # return the top level path
+                    return os.path.join(orig_path, CARD_SUFFIX)
+                current_path = new_path
                 check_dir = os.path.join(current_path, local_dir)
-                check_dir = os.path.realpath(check_dir)
-                orig_path = check_dir
-                while not os.path.isdir(check_dir):
-                    new_path = os.path.dirname(current_path)
-                    if new_path == current_path:
-                        # No longer making upward progress so we
-                        # return the top level path
-                        return os.path.join(orig_path, CARD_SUFFIX)
-                    current_path = new_path
-                    check_dir = os.path.join(current_path, local_dir)
-                return os.path.join(check_dir, CARD_SUFFIX)
+            return os.path.join(check_dir, CARD_SUFFIX)
         else:
             # Let's make it obvious we need to update this block for each new datastore backend...
             raise NotImplementedError(

test/core/test_card_local_storage.py

@@ -0,0 +1,166 @@
+"""Regression coverage for local card storage root consistency (issue #2139).
+
+Background
+----------
+Issue #2139 reported that, with a configured local datastore sysroot, flow
+artifacts and `@card` HTML ended up in *different* directories: artifacts under
+`METAFLOW_DATASTORE_SYSROOT_LOCAL`, but cards under a `.metaflow/mf.cards`
+directory created relative to the current working directory. The cards were
+therefore "missing" from the location the user mounted into their UI pod.
+
+Why this is an end-to-end subprocess test
+------------------------------------------
+The bug lives in the seam between the *writer* and the *reader*, not inside a
+single function:
+
+  * the writer is a separate ``<flow>.py card create`` subprocess spawned by the
+    card decorator during the run, and
+  * the reader (``card list`` / the client) resolves the card root from the
+    task's persisted ``ds-root`` metadata.
+
+A unit test on ``CardDatastore.get_storage_root`` can prove the resolved path is
+correct, but only a real run can prove that a card written by the subprocess is
+found again through the public CLI. This test crosses that boundary on purpose.
+
+The unit-level coverage for each resolution branch lives in
+``test/unit/test_card_datastore.py``.
+"""
+
+import json
+import os
+from pathlib import Path
+import subprocess
+import sys
+import textwrap
+
+
+REPOSITORY_ROOT = Path(__file__).resolve().parents[2]
+
+
+def test_card_round_trip_uses_configured_local_datastore_root(tmp_path):
+    """Cards must land under the configured datastore sysroot and be readable.
+
+    Fails before the fix (cards are written under ``<cwd>/.metaflow/mf.cards``),
+    passes after it (cards are written under
+    ``<sysroot>/.metaflow/mf.cards`` and the ``card list`` reader finds them).
+    """
+    # `work_dir` is the cwd we run the flow from; it must stay free of a
+    # `.metaflow` directory. `shared_root` is the explicitly configured
+    # datastore sysroot where BOTH artifacts and cards are expected to land.
+    work_dir = tmp_path / "work"
+    shared_root = tmp_path / "shared"
+    # Isolated METAFLOW_HOME so a developer's ~/.metaflowconfig cannot influence
+    # the run (e.g. flip metadata to a live service).
+    config_home = tmp_path / "config"
+    work_dir.mkdir()
+    config_home.mkdir()
+
+    flow_file = work_dir / "card_root_regression_flow.py"
+    flow_file.write_text(
+        textwrap.dedent(
+            """
+            from metaflow import FlowSpec, card, step
+
+
+            class CardRootRegressionFlow(FlowSpec):
+                @card(type="blank")
+                @step
+                def start(self):
+                    self.value = "round-trip"
+                    self.next(self.end)
+
+                @step
+                def end(self):
+                    print(self.value)
+
+
+            if __name__ == "__main__":
+                CardRootRegressionFlow()
+            """
+        ).lstrip()
+    )
+
+    env = os.environ.copy()
+    pythonpath = env.get("PYTHONPATH")
+    env.update(
+        {
+            "METAFLOW_DEFAULT_DATASTORE": "local",
+            "METAFLOW_DEFAULT_METADATA": "local",
+            "METAFLOW_DATASTORE_SYSROOT_LOCAL": str(shared_root),
+            "METAFLOW_HOME": str(config_home),
+            "PYTHONPATH": (
+                "%s:%s" % (REPOSITORY_ROOT, pythonpath)
+                if pythonpath
+                else str(REPOSITORY_ROOT)
+            ),
+        }
+    )
+    # Keep the test deterministic: no explicit card root (that path is covered
+    # by unit tests), and no inherited profile/service settings.
+    env.pop("METAFLOW_CARD_LOCALROOT", None)
+    env.pop("METAFLOW_PROFILE", None)
+    env.pop("METAFLOW_SERVICE_URL", None)
+
+    # Writer side: run the flow. The @card decorator spawns a separate
+    # `card create` subprocess, so this exercises the real write path.
+    run_result = subprocess.run(
+        [sys.executable, str(flow_file), "run"],
+        cwd=str(work_dir),
+        env=env,
+        capture_output=True,
+        text=True,
+        timeout=30,
+    )
+    assert run_result.returncode == 0, run_result.stdout + run_result.stderr
+
+    # `local` datastore stores everything under `<sysroot>/.metaflow`, and cards
+    # belong under the `mf.cards` sibling of the flow artifacts.
+    datastore_root = shared_root / ".metaflow"
+    card_root = datastore_root / "mf.cards"
+    card_files = list(card_root.rglob("*.html"))
+
+    # Core assertion for #2139: the card was written under the configured root...
+    assert card_files, (
+        "Expected cards below configured datastore root %s.\n"
+        "Flow output:\n%s%s"
+        % (card_root, run_result.stdout, run_result.stderr)
+    )
+    # ...and NOT in a stray `.metaflow` created next to the flow script.
+    assert not (work_dir / ".metaflow").exists()
+
+    # Derive the run/task ids from what actually ran rather than hardcoding them
+    # (task ids are not guaranteed to be a fixed value).
+    run_id = (
+        datastore_root / "CardRootRegressionFlow" / "latest_run"
+    ).read_text().strip()
+    # Card path layout: .../tasks/<task_id>/cards/<file>.html, so the task id is
+    # two levels above the HTML file.
+    task_id = card_files[0].parents[1].name
+
+    # Reader side: the client/CLI resolves the card root from persisted `ds-root`
+    # metadata. This proves the reader looks where the writer actually wrote.
+    list_result = subprocess.run(
+        [
+            sys.executable,
+            str(flow_file),
+            "--quiet",
+            "card",
+            "list",
+            "%s/start/%s" % (run_id, task_id),
+            "--as-json",
+        ],
+        cwd=str(work_dir),
+        env=env,
+        capture_output=True,
+        text=True,
+        timeout=30,
+    )
+
+    assert list_result.returncode == 0, list_result.stdout + list_result.stderr
+    listed_cards = json.loads(list_result.stdout)
+    # The reader resolved the same task and found the single `blank` card the
+    # writer produced -> write and read paths agree.
+    assert listed_cards["pathspec"] == (
+        "CardRootRegressionFlow/%s/start/%s" % (run_id, task_id)
+    )
+    assert [card["type"] for card in listed_cards["cards"]] == ["blank"]

test/unit/test_card_datastore.py

@@ -0,0 +1,142 @@
+"""Unit coverage for card datastore root resolution (issue #2139).
+
+``CardDatastore.get_storage_root`` decides where local/spin cards are written and
+read. These tests pin the resolution order the fix guarantees:
+
+  1. an explicit ``METAFLOW_CARD_LOCALROOT`` wins;
+  2. otherwise cards sit under the resolved datastore root (``<root>/mf.cards``),
+     so they stay next to the flow artifacts;
+  3. otherwise fall back to the legacy nearest-parent ``.metaflow`` lookup, then
+     to the current working directory.
+
+Cloud backends (s3/azure/gs) must be unaffected. The end-to-end write/read
+round-trip lives in ``test/core/test_card_local_storage.py``.
+"""
+
+import os
+
+import pytest
+
+from metaflow.metaflow_config import (
+    CARD_SUFFIX,
+    DATASTORE_LOCAL_DIR,
+    DATASTORE_SPIN_LOCAL_DIR,
+)
+from metaflow.plugins.cards import card_datastore
+from metaflow.plugins.cards.card_datastore import CardDatastore
+
+
+@pytest.fixture
+def no_explicit_card_root(monkeypatch):
+    monkeypatch.setattr(card_datastore, "CARD_LOCALROOT", None)
+
+
+def test_local_card_root_honors_explicit_config(monkeypatch, tmp_path):
+    # Priority 1: an explicit card root must win even when a datastore root is
+    # also supplied. (Before the fix this returned None and was silently ignored.)
+    explicit_card_root = tmp_path / "cards"
+    datastore_root = tmp_path / "shared" / DATASTORE_LOCAL_DIR
+    monkeypatch.setattr(card_datastore, "CARD_LOCALROOT", str(explicit_card_root))
+
+    assert CardDatastore.get_storage_root(
+        "local", datastore_root=str(datastore_root)
+    ) == str(explicit_card_root)
+
+
+def test_local_card_root_uses_resolved_datastore_root(
+    no_explicit_card_root, monkeypatch, tmp_path
+):
+    # Priority 2 (the #2139 fix): with no explicit card root, cards follow the
+    # resolved datastore root instead of the current working directory. The cwd
+    # must NOT gain a `.metaflow` directory.
+    work_dir = tmp_path / "work"
+    datastore_root = tmp_path / "shared" / DATASTORE_LOCAL_DIR
+    work_dir.mkdir()
+    monkeypatch.chdir(work_dir)
+
+    assert CardDatastore.get_storage_root(
+        "local", datastore_root=str(datastore_root)
+    ) == os.path.join(str(datastore_root), CARD_SUFFIX)
+    assert not (work_dir / DATASTORE_LOCAL_DIR).exists()
+
+
+def test_local_card_root_falls_back_to_nearest_metaflow_directory(
+    no_explicit_card_root, monkeypatch, tmp_path
+):
+    # Priority 3, legacy behavior: with neither an explicit card root nor a
+    # supplied datastore root, walk upward to the nearest existing `.metaflow`.
+    ancestor = tmp_path / "ancestor"
+    work_dir = ancestor / "project" / "subdir"
+    metaflow_dir = ancestor / DATASTORE_LOCAL_DIR
+    work_dir.mkdir(parents=True)
+    metaflow_dir.mkdir()
+    monkeypatch.chdir(work_dir)
+
+    assert CardDatastore.get_storage_root("local") == os.path.join(
+        str(metaflow_dir.resolve()), CARD_SUFFIX
+    )
+
+
+def test_local_card_root_treats_empty_datastore_root_as_missing(
+    no_explicit_card_root, monkeypatch, tmp_path
+):
+    # Keep compatibility with older/empty `ds-root` metadata: an empty string is
+    # not a valid configured root, so fall through to the legacy lookup.
+    ancestor = tmp_path / "ancestor"
+    work_dir = ancestor / "project" / "subdir"
+    metaflow_dir = ancestor / DATASTORE_LOCAL_DIR
+    work_dir.mkdir(parents=True)
+    metaflow_dir.mkdir()
+    monkeypatch.chdir(work_dir)
+
+    assert CardDatastore.get_storage_root("local", datastore_root="") == os.path.join(
+        str(metaflow_dir.resolve()), CARD_SUFFIX
+    )
+
+
+def test_local_card_root_falls_back_to_cwd_when_no_root_exists(
+    no_explicit_card_root, monkeypatch, tmp_path
+):
+    # Legacy last resort: no config, no supplied root, and no ancestor
+    # `.metaflow` -> use `<cwd>/.metaflow/mf.cards`.
+    work_dir = tmp_path / "work"
+    work_dir.mkdir()
+    monkeypatch.chdir(work_dir)
+
+    assert CardDatastore.get_storage_root("local") == os.path.join(
+        str((work_dir / DATASTORE_LOCAL_DIR).resolve()), CARD_SUFFIX
+    )
+
+
+def test_spin_card_root_uses_resolved_datastore_root(no_explicit_card_root, tmp_path):
+    # spin shares this code path with local; guard against fixing local while
+    # regressing spin. spin uses `.metaflow_spin` as its datastore dir.
+    datastore_root = tmp_path / DATASTORE_SPIN_LOCAL_DIR
+
+    assert CardDatastore.get_storage_root(
+        "spin", datastore_root=str(datastore_root)
+    ) == os.path.join(str(datastore_root), CARD_SUFFIX)
+
+
+@pytest.mark.parametrize(
+    "storage_type,root_attr",
+    [
+        ("s3", "CARD_S3ROOT"),
+        ("azure", "CARD_AZUREROOT"),
+        ("gs", "CARD_GSROOT"),
+    ],
+)
+def test_cloud_card_roots_are_unchanged(
+    monkeypatch, tmp_path, storage_type, root_attr
+):
+    # The fix is scoped to local/spin. Cloud backends must keep returning their
+    # configured root and ignore any supplied datastore_root.
+    configured_root = "cloud://bucket/%s" % storage_type
+    monkeypatch.setattr(card_datastore, root_attr, configured_root)
+
+    assert (
+        CardDatastore.get_storage_root(
+            storage_type, datastore_root=str(tmp_path / "ignored")
+        )
+        == configured_root
+    )