Skip to content

Commit 34f2de8

Browse files
KnutZuidematalsperreclaude
authored
fix: skip CronWorkflow creation when @schedule resolves to no schedule (#3122)
## PR Type - [x] Bug fix ## Summary When `@schedule` is present but all scheduling flags are falsy (e.g. `@schedule(daily=False)`), deploying to Argo Workflows crashes with `AttributeError: 'NoneType' object has no attribute 'split'` and no WorkflowTemplate is created. This fix makes the deploy succeed and skip CronWorkflow creation, which is the expected behaviour when no schedule is configured. ## Issue Fixes #3121 ## Reproduction **Runtime:** argo **Commands to run:** ```bash python myflow.py argo-workflows create ``` Where `myflow.py` contains: ```python from metaflow import FlowSpec, step, schedule @schedule(daily=False) class MyFlow(FlowSpec): @step def start(self): self.next(self.end) @step def end(self): pass if __name__ == "__main__": MyFlow() ``` **Where evidence shows up:** parent console / `argo-workflows create` output <details> <summary>Before (error / log snippet)</summary> ``` AttributeError: 'NoneType' object has no attribute 'split' File ".../metaflow/plugins/argo/argo_workflows.py", line 463, in _get_schedule return " ".join(schedule.schedule.split()[:5]), schedule.timezone ``` </details> <details> <summary>After (evidence that fix works)</summary> ``` Workflow template 'MyFlow' created successfully. No CronWorkflow created (no schedule configured). ``` </details> ## Root Cause `ScheduleDecorator.flow_init` sets `self.schedule = None` when all scheduling flags are falsy (`schedule_decorator.py:49`). `_get_schedule()` in `argo_workflows.py` guarded against the decorator being absent (`if schedule:`), but not against `schedule.schedule` being `None`. Calling `.split()` on `None` raises `AttributeError`. Separately, `trigger_explanation()` checked decorator presence rather than `self._schedule`, so it would incorrectly report CronWorkflow-based triggering even when no schedule was resolved. ## Why This Fix Is Correct `_get_schedule()` now returns `(None, None)` when `schedule.schedule is None`, treating it identically to the decorator being absent. The rest of the deploy path already handles `self._schedule is None` correctly — no CronWorkflow is created. `trigger_explanation()` now checks `self._schedule is not None`, which is the actual resolved value, rather than decorator presence. ## Failure Modes Considered 1. **Flows with a real schedule are unaffected** — the `schedule.schedule is None` guard is only entered when all flags are falsy; any valid cron string, `daily=True`, `hourly=True`, or `weekly=True` sets a non-None value and takes the existing return path. 2. **Backward compatibility** — the previous behaviour for the `@schedule(daily=False)` case was a crash, so changing it to a clean no-op is strictly an improvement with no risk of regression. ## Tests - [x] Unit tests added/updated - [x] Reproduction script provided (required for Core Runtime) - [ ] CI passes - [ ] If tests are impractical: explain why below and provide manual evidence above The bug is straightforward to reproduce with the script above. Unit test coverage for `_get_schedule()` with a `None` schedule value would be a good addition. ## Non-Goals This fix does not address the Step Functions path (which returns `schedule.schedule` directly and handles `None` safely already), does not implement an `--no-enable-schedule` flag (see PR #3102), and does not change any scheduling behaviour for flows where a schedule is actually configured. ## AI Tool Usage - [x] AI tools were used (describe below) OpenCode (Claude Sonnet) was used to identify the bug, draft the issue, and generate the fix. All generated code was reviewed and understood before committing. --------- Co-authored-by: Shashank Srikanth <ssrikanth@netflix.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent fc8f853 commit 34f2de8

2 files changed

Lines changed: 111 additions & 1 deletion

File tree

metaflow/plugins/argo/argo_workflows.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,11 @@ def _get_schedule(self):
460460
if schedule:
461461
# Remove the field "Year" if it exists
462462
schedule = schedule[0]
463+
if schedule.schedule is None:
464+
# @schedule decorator is present but all scheduling flags are
465+
# falsy (e.g. @schedule(daily=False)). Treat this the same as
466+
# no decorator — do not create a CronWorkflow.
467+
return None, None
463468
return " ".join(schedule.schedule.split()[:5]), schedule.timezone
464469
return None, None
465470

@@ -482,7 +487,7 @@ def schedule(self):
482487

483488
def trigger_explanation(self):
484489
# Trigger explanation for cron workflows
485-
if self.flow._flow_decorators.get("schedule"):
490+
if self._schedule is not None:
486491
return (
487492
"This workflow triggers automatically via the CronWorkflow *%s*."
488493
% self.name

test/unit/test_argo_workflows_cli.py

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1+
import types
2+
13
import pytest
24

35
from metaflow.plugins.argo.argo_workflows_cli import sanitize_for_argo
6+
from metaflow.plugins.argo.argo_workflows import ArgoWorkflows
47

58

69
@pytest.mark.parametrize(
@@ -27,3 +30,105 @@
2730
def test_sanitize_for_argo(name, expected):
2831
sanitized = sanitize_for_argo(name)
2932
assert sanitized == expected
33+
34+
35+
@pytest.fixture
36+
def make_argo_with_schedule():
37+
"""
38+
Factory fixture: returns a callable that builds a minimal ArgoWorkflows-like
39+
object whose _get_schedule() can be called without instantiating the full
40+
class (which requires a live graph, environment, datastore, etc.).
41+
42+
The method only accesses self.flow._flow_decorators, so we build the
43+
smallest possible stand-in for each layer.
44+
"""
45+
46+
def _make(schedule_value, timezone_value=None):
47+
decorator = types.SimpleNamespace(
48+
schedule=schedule_value, timezone=timezone_value
49+
)
50+
flow = types.SimpleNamespace(_flow_decorators={"schedule": [decorator]})
51+
instance = object.__new__(ArgoWorkflows)
52+
instance.flow = flow
53+
return instance
54+
55+
return _make
56+
57+
58+
@pytest.fixture
59+
def argo_without_schedule():
60+
"""ArgoWorkflows stand-in with no @schedule decorator at all."""
61+
flow = types.SimpleNamespace(_flow_decorators={})
62+
instance = object.__new__(ArgoWorkflows)
63+
instance.flow = flow
64+
return instance
65+
66+
67+
def test_get_schedule_no_decorator_returns_none(argo_without_schedule):
68+
"""No @schedule decorator → (None, None)."""
69+
assert argo_without_schedule._get_schedule() == (None, None)
70+
71+
72+
@pytest.mark.parametrize(
73+
"schedule_value, timezone_value, expected",
74+
[
75+
# schedule resolved to None → (None, None) regardless of timezone.
76+
# The schedule_none case is the regression test for the AttributeError
77+
# bug where None.split() was called when all scheduling flags were falsy.
78+
(None, None, (None, None)),
79+
(None, "America/Los_Angeles", (None, None)),
80+
# 6-field quartz cron → trimmed to 5 fields, timezone passed through.
81+
("0 0 * * ? *", None, ("0 0 * * ?", None)),
82+
("0 0 * * ? *", "Europe/Berlin", ("0 0 * * ?", "Europe/Berlin")),
83+
],
84+
ids=[
85+
"schedule_none",
86+
"schedule_none_with_timezone",
87+
"cron_expression",
88+
"cron_expression_with_timezone",
89+
],
90+
)
91+
def test_get_schedule(
92+
make_argo_with_schedule, schedule_value, timezone_value, expected
93+
):
94+
argo = make_argo_with_schedule(
95+
schedule_value=schedule_value, timezone_value=timezone_value
96+
)
97+
assert argo._get_schedule() == expected
98+
99+
100+
def test_trigger_explanation_no_schedule_does_not_claim_cronworkflow(
101+
argo_without_schedule,
102+
):
103+
"""With no schedule, trigger_explanation() must not mention CronWorkflow."""
104+
argo_without_schedule._schedule = None
105+
argo_without_schedule.triggers = []
106+
result = argo_without_schedule.trigger_explanation()
107+
assert "CronWorkflow" not in result
108+
109+
110+
def test_trigger_explanation_schedule_none_does_not_claim_cronworkflow(
111+
make_argo_with_schedule,
112+
):
113+
"""
114+
When @schedule is present but resolved to None, trigger_explanation()
115+
must not claim the workflow triggers via a CronWorkflow.
116+
"""
117+
argo = make_argo_with_schedule(schedule_value=None)
118+
argo._schedule = None # mirrors what _get_schedule() would set
119+
argo.triggers = []
120+
result = argo.trigger_explanation()
121+
assert "CronWorkflow" not in result
122+
123+
124+
def test_trigger_explanation_active_schedule_claims_cronworkflow(
125+
make_argo_with_schedule,
126+
):
127+
"""When a real schedule is set, trigger_explanation() names the CronWorkflow."""
128+
argo = make_argo_with_schedule(schedule_value="0 0 * * ? *")
129+
argo._schedule = "0 0 * * ?"
130+
argo.name = "myflow"
131+
result = argo.trigger_explanation()
132+
assert result is not None
133+
assert "CronWorkflow" in result
134+
assert "myflow" in result

0 commit comments

Comments
 (0)