fix: swap operand order so _metaflow parameter is respected in Metafl…

[khushthecoder]

fix: swap operand order so _metaflow parameter is respected in MetaflowObject.__init__

Fixes #3019

PR Type

• Core Runtime change (higher bar -- see CONTRIBUTING.md)

Summary

_metaflow parameter passed to MetaflowObject.__init__ was silently discarded due to wrong or operand order, causing every child object to allocate a new Metaflow() from the global provider instead of reusing the parent's. This broke cross-environment unpickling where children queried a different metadata backend than the parent.

Issue

Fixes #3019

Reproduction

Runtime: local

Commands to run:

python3 -c "
from unittest.mock import patch, MagicMock
from metaflow.client.core import Flow, Metaflow

# Create a Metaflow instance to pass in
class DummyMF:
    def __init__(self):
        self.metadata = MagicMock()
        self.metadata.get_object.return_value = {
            'flow_id': 'TestFlow', 'ts_epoch': 1700000000000,
            'tags': [], 'system_tags': [],
        }
    def __bool__(self): return True

passed_mf = DummyMF()

with patch.object(Flow, '_get_object', return_value={
    'flow_id': 'TestFlow', 'ts_epoch': 1700000000000,
    'tags': [], 'system_tags': [],
}):
    flow = Flow('TestFlow', _metaflow=passed_mf, _namespace_check=False)

print('_metaflow is passed instance:', flow._metaflow is passed_mf)
print('type(flow._metaflow):', type(flow._metaflow).__name__)
"

Where evidence shows up: parent console

Before (error / log snippet)

_metaflow is passed instance: False
type(flow._metaflow): Metaflow

The passed _metaflow is discarded; a fresh Metaflow() is constructed instead.

After (evidence that fix works)

_metaflow is passed instance: True
type(flow._metaflow): DummyMF

The passed _metaflow is now correctly used.

Root Cause

In metaflow/client/core.py, line 288 inside MetaflowObject.__init__:

self._metaflow = Metaflow(_current_metadata) or _metaflow

Python's or operator returns the first truthy value. Metaflow(_current_metadata) always returns a truthy object because its __init__ (lines 2619–2628) unconditionally assigns self.metadata to a real provider — either via _metadata() or from current_metadata. Since the left operand is always truthy, or _metaflow is never evaluated.

Cascading consequences:

1. Every child object construction in __iter__ (line 409), __getitem__ (line 525), and Metaflow.__iter__ (line 665) passes _metaflow=self._metaflow, but it's silently ignored.
2. Cross-environment unpickling via _unpickle_21227 restores the parent's metadata via _current_metadata, but when the parent iterates children, those children pass _metaflow=self._metaflow which is discarded — children fall back to the global provider, potentially querying a different backend.

Why This Fix Is Correct

Swapping the operands to _metaflow or Metaflow(_current_metadata) restores the intended invariant: use the caller-supplied _metaflow when available, only construct a new one as fallback. The fix is minimal — a single operand swap — and preserves all existing behavior for the _metaflow=None (default) case since None or Metaflow(...) still constructs a new instance.

Failure Modes Considered

1. Backward compatibility for _metaflow=None (default path): When no _metaflow is passed (the common case for top-level Flow('name') or Run('name/id') calls), None or Metaflow(_current_metadata) still constructs a new Metaflow() — identical behavior to before.

2. Truthy _metaflow skipping _current_metadata: When _metaflow is passed (internal code paths: __iter__, __getitem__, unpickling), _current_metadata is not used. This is correct because the parent already has the right metadata provider — constructing a new one would be wasteful and potentially inconsistent.

3. Falsy _metaflow edge case: If someone were to pass a falsy non-None value for _metaflow, it would fall through to construct Metaflow(_current_metadata). All existing call sites pass either None (default) or self._metaflow (always truthy), so this path is never hit in practice but fails safe.

Tests

• Unit tests added/updated
• Reproduction script provided (required for Core Runtime)
• CI passes
• If tests are impractical: explain why below and provide manual evidence above

5 tests added in test/unit/test_metaflow_object_init.py:

• test_passed_metaflow_is_used — verifies self._metaflow is passed_mf (the core bug)
• test_no_metaflow_falls_back_to_new_instance — verifies default path still works
• test_child_inherits_parent_metaflow — verifies Run('X/42', _metaflow=mf) holds the parent's instance
• test_passed_metaflow_takes_precedence_over_current_metadata — verifies _metaflow wins when both are given
• test_getitem_passes_parent_metaflow — verifies flow["99"] propagates _metaflow to child

Non-Goals

• No changes to Metaflow.__init__, _unpickle_* methods, or __getstate__/__setstate__ — those are correct as-is.
• No changes to the metadata provider layer.
• No new features or API changes.

AI Tool Usage

• AI tools were used (describe below)

• Tool: Antigravity (AI coding assistant)

• Used for: Identifying the fix location, generating the test scaffolding, and drafting the PR description

• All generated code was reviewed, understood, and tested manually. I can explain every line of the fix and tests independently.

[greptile-apps]

Greptile Summary

This PR fixes a silent bug in MetaflowObject.__init__ where the caller-supplied _metaflow parameter was always discarded because the or operands were in the wrong order — Metaflow(_current_metadata) is always truthy, so _metaflow on the right-hand side was never evaluated. The fix is a single-character-level operand swap on line 288 of metaflow/client/core.py.

Key changes:

• metaflow/client/core.py line 288: Metaflow(_current_metadata) or _metaflow → _metaflow or Metaflow(_current_metadata). This restores the intended invariant: prefer the caller-supplied instance, fall back to constructing a new one only when none is given.
• test/unit/test_metaflow_object_init.py: Five regression tests are added covering the core bug, the default fallback path, direct _metaflow storage on Run, precedence over _current_metadata, and __getitem__ propagation to children.

The fix is minimal, backward-compatible (the _metaflow=None default path behaves identically), and directly resolves the cross-environment unpickling scenario described in issue #3019. One minor gap exists: __iter__ propagation is not covered by the new tests (only __getitem__ is), which leaves a small regression surface uncovered.

Confidence Score: 5/5

• This PR is safe to merge — the fix is a minimal, well-understood one-line operand swap with no risk of regressions on the default code path.
• The change is a single operand swap that is trivially correct: Python's or short-circuits on the first truthy value, and None or Metaflow(...) preserves the existing default behavior exactly. The new tests validate both the fixed and the default paths. The only gap is a missing __iter__ propagation test, which is a coverage concern rather than a correctness concern.
• No files require special attention.

Important Files Changed

Filename	Overview
metaflow/client/core.py	Single-line operand swap fixing the _metaflow parameter being silently discarded; fix is correct, minimal, and backward-compatible.
test/unit/test_metaflow_object_init.py	Five new regression tests covering the key scenarios; tests are sound and use proper pytest-style patterns consistent with the rest of the test suite, with a minor gap around __iter__ propagation coverage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["MetaflowObject.__init__(pathspec, _metaflow, _current_metadata)"] --> B{"_metaflow is truthy?"}
    B -- "Yes (caller supplied instance)" --> C["self._metaflow = _metaflow\n(reuse parent's provider)"]
    B -- "No (_metaflow is None / default)" --> D["self._metaflow = Metaflow(_current_metadata)\n(construct new provider)"]
    C --> E["Continue __init__\n(namespace, tags, id resolution)"]
    D --> E
    E --> F["__iter__ / __getitem__\npasses self._metaflow\nto child objects"]
    F --> G["Child MetaflowObject\nreuses same metadata backend"]

    style C fill:#d4edda,stroke:#28a745
    style D fill:#fff3cd,stroke:#ffc107

Loading

_{Last reviewed commit: 2e17951}

[greptile-apps]

__iter__ propagation not tested

test_getitem_passes_parent_metaflow verifies the __getitem__ path, but there is no corresponding test for the __iter__ path (lines 394–416 of core.py), which is the other major call site that passes _metaflow=self._metaflow to child objects. The two paths are symmetric, so a bug regressing one without the other could go undetected.

Consider adding a test that patches Flow.__iter__ (or metadata.get_object used inside __iter__) and verifies that each yielded child carries the parent's _metaflow instance:

def test_iter_passes_parent_metaflow(self):
    """Flow.__iter__ should propagate _metaflow to every yielded child."""
    parent_mf = _DummyMetaflow()
    child_obj = {
        "run_number": "7",
        "ts_epoch": 1700000000000,
        "tags": [],
        "system_tags": [],
    }

    from metaflow.client.core import Flow

    with patch.object(Flow, "_get_object", return_value={
        "flow_id": "TestFlow",
        "ts_epoch": 1700000000000,
        "tags": [],
        "system_tags": [],
    }):
        flow = Flow(pathspec="TestFlow", _metaflow=parent_mf, _namespace_check=False)

    # Patch metadata.get_object so __iter__ yields one child
    parent_mf.metadata.get_object.return_value = [child_obj]
    children = list(flow)
    assert len(children) == 1
    assert children[0]._metaflow is parent_mf

[khushthecoder]

hi @savingoyal @saikonen I have submitted a PR addressing #3019 issue. Could you please review it? If any changes are required, I am available to make them.