Skip to content

Normalize empty worker namespace in step commands#3063

Open
sahar-saidani wants to merge 1 commit into
Netflix:masterfrom
sahar-saidani:fix-2734-global-namespace
Open

Normalize empty worker namespace in step commands#3063
sahar-saidani wants to merge 1 commit into
Netflix:masterfrom
sahar-saidani:fix-2734-global-namespace

Conversation

@sahar-saidani
Copy link
Copy Markdown

@sahar-saidani sahar-saidani commented Mar 30, 2026

Fixes #2734

PR Type

  • Bug fix
  • New feature
  • Core Runtime change (higher bar -- see CONTRIBUTING.md)
  • Docs / tooling
  • Refactoring

Summary

This fixes a namespace regression in the worker-side step and spin_step paths. When namespace(None) is used to select the global namespace, the worker could receive an empty string instead, which caused client access inside the worker to fail with Object not in namespace ''.

Issue

Fixes #2734

Reproduction

Runtime: local

Commands to run:

python -m pytest test/unit/spin/test_spin.py -v -k global_namespace

Where evidence shows up: parent console / task logs from the local runtime

<details>
<summary>Before (error / log snippet)</summary>
test/unit/spin/test_spin.py::test_spin_preserves_global_namespace_end_to_end FAILED
Running global namespace regression test for /mnt/c/Users/ASUS/Documents/
Projets_Personnel/metaflow/test/unit/spin/flows/global_namespace_flow.py
Metaflow 2.19.22-dirty executing GlobalNamespaceFlow for user:sahar Validating your flow...
    The graph looks good!
Running pylint...
    Pylint not found, so extra checks are disabled.
2026-03-30 14:13:10.524 Workflow starting (run-id 1774876390494017):
2026-03-30 14:13:10.932 [1774876390494017/start/1 (pid 949)] Task is starting.
2026-03-30 14:13:15.475 [1774876390494017/start/1 (pid 949)] <flow GlobalNamespaceFlow step start> failed:
2026-03-30 14:13:15.738 [1774876390494017/start/1 (pid 949)] Object not in the current namespace:
2026-03-30 14:13:15.773 [1774876390494017/start/1 (pid 949)] Object not in namespace ''
2026-03-30 14:13:15.785 [1774876390494017/start/1 (pid 949)] Task failed.
2026-03-30 14:13:15.800 Workflow failed.
</details>

<details>
<summary>After (evidence that fix works)</summary>
=========================== test session starts============================
platform linux -- Python 3.12.3, pytest-9.0.2, pluggy-1.6.0 -- /mnt/c/
Users/ASUS/Documents/Projets_Personnel/metaflow/.venv/bin/python
cachedir: .pytest_cache
rootdir:/mnt/c/Users/ASUS/Documents/Projets_Personnel/metaflow
collected 13 items / 12 deselected / 1 selected
test/unit/spin/test_spin.py::test_spin_preserves_global_namespace_end_to_end PASSED [100%]
=============== 1 passed, 12 deselected, 1 warning in 24.81s ===============
</details>

Root Cause

The runtime forwards the current namespace to worker-side commands in the step and spin-step paths using get_namespace() or "".
That preserves explicit namespaces, but it changes the meaning of the global namespace case. namespace(None) is intended to mean "no namespace
filtering", but once it reaches the worker as "", the worker-side code applies namespace(opt_namespace) and treats the empty string as a real namespace value instead of restoring None.
That breaks worker-side client access and leads to Object not in namespace ' ' .

Why This Fix Is Correct

This change restores the intended behavior by normalizing the empty worker-side namespace value back to None.

The fix is intentionally small:
1- it keeps the runtime-side behavior unchanged
2- it normalizes the namespace only at the worker boundary
3- it applies the same fix in both step and spin_step

Explicit non-empty namespaces continue to behave the same way.

Failure Modes Considered

  1. Explicit namespaces should remain unchanged. This fix only affects the empty-value case and preserves non-empty namespace values as-is.
  2. Both the normal runtime path and the spin path need to be covered. The regression test exercises both.

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

Added an end-to-end regression test covering the real runtime and spin paths.

Executed:

python -m pytest test/unit/spin/test_spin.py -v -k global_namespace

I also ran:

python -m pytest test/unit/spin/test_spin.py -v

The full test_spin.py suite is blocked in my local environment by unrelated setup issues:

  • complex_dag_flow.py fails during conda/micromamba resolution because scikit-learn==1.3.0 is incompatible with Python 3.12 in that environment
  • test_card_flow requires pandas, which is not installed in my local environment

Non-Goals

This PR does not change broader namespace handling in the runtime, client, or metadata layers. It only restores the intended worker-side behavior for the empty-value case in step and spin_step.

AI Tool Usage

  • No AI tools were used in this contribution
  • AI tools were used (describe below)

I used AI assistance during investigation and drafting. I reviewed, understood, and tested the final changes myself.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 30, 2026

Greptile Summary

This PR fixes a namespace regression in the worker-side step and spin_step paths: when namespace(None) (global namespace) is active, runtime.py forwards it as get_namespace() or "", and the workers were calling namespace("") instead of namespace(None), causing Object not in namespace '' errors. The fix is a two-character change — namespace(opt_namespace or None) — applied symmetrically in both paths.

Key changes:

  • metaflow/cli_components/step_cmd.py: opt_namespace or None normalises the empty-string sentinel back to None before calling namespace(), in both step (line 125) and spin_step (line 279).
  • test/unit/spin/flows/global_namespace_flow.py: New regression-flow that exercises Run(...) client access from inside a step when namespace(None) is set at module scope.
  • test/unit/spin/test_spin.py: New end-to-end regression test that verifies the fix on both the normal runtime path and the spin path.

Minor issues found:

  • The new test function body uses 6-space indentation instead of 4-space (the file-wide convention), and is separated from the previous function by only one blank line instead of two (PEP 8).
  • global_namespace_flow.py is missing a trailing newline at EOF.

Confidence Score: 5/5

Safe to merge — the fix is correct, minimal, and well-targeted; remaining findings are PEP 8 style nits.

The core change is a minimal, surgically correct two-line fix that is well-understood (the or "" sentinel in runtime.py → or None normalisation in the worker). Both affected paths are covered. All remaining comments are P2 style issues (indentation, blank lines, missing EOF newline) that do not affect correctness or runtime behaviour.

No files require special attention beyond the minor style fixes in test/unit/spin/test_spin.py and test/unit/spin/flows/global_namespace_flow.py.

Important Files Changed

Filename Overview
metaflow/cli_components/step_cmd.py Correct, minimal fix: opt_namespace or None normalizes the empty-string sentinel (produced by get_namespace() or "" in runtime.py) back to None in both step and spin_step, restoring global-namespace semantics in the worker.
test/unit/spin/flows/global_namespace_flow.py New regression-test flow that reproduces #2734; minor: missing trailing newline at EOF.
test/unit/spin/test_spin.py New end-to-end regression test covering both runtime and spin paths; minor: 6-space indentation and only one blank line before the new function violate PEP 8 and the file's existing style.

Reviews (1): Last reviewed commit: "Normalize empty worker namespace in step..." | Re-trigger Greptile

Comment thread test/unit/spin/test_spin.py
Comment thread test/unit/spin/flows/global_namespace_flow.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Namespace declarition outside step won't work on Metaflow 2.19.15

1 participant