|
1 | | -"""Tests for @resources vs @kubernetes decorator precedence logic. |
| 1 | +"""Tests for @resources vs @kubernetes precedence logic. |
2 | 2 |
|
3 | | -The merge logic in KubernetesDecorator.step_init works as follows: |
4 | | -- If @kubernetes attribute is at its default AND @resources explicitly sets a value |
5 | | - → use the @resources value (even if lower than the @kubernetes default). |
6 | | -- If both explicitly set → take max. |
7 | | -- If both at default → take max (which equals default). |
| 3 | +The merge logic in KubernetesDecorator.step_init determines which value |
| 4 | +wins when both @resources and @kubernetes set the same attribute (e.g. memory). |
| 5 | +
|
| 6 | +Rules: |
| 7 | +- If @kubernetes is at its default and @resources explicitly sets a value, |
| 8 | + @resources wins (even if lower). |
| 9 | +- If both are explicitly set, the larger value wins. |
| 10 | +- If both are at default, the default is kept. |
8 | 11 | """ |
9 | 12 |
|
10 | | -K8S_DEFAULTS = {"cpu": "1", "memory": "4096", "disk": "10240"} |
11 | | -RESOURCES_DEFAULTS = {"cpu": "1", "memory": "4096"} |
| 13 | +from metaflow.plugins.kubernetes.kubernetes_decorator import KubernetesDecorator |
| 14 | +from metaflow.plugins.resources_decorator import ResourcesDecorator |
| 15 | + |
| 16 | + |
| 17 | +def _merge_resource(k8s_val, resources_val, attr="memory"): |
| 18 | + """Simulate the merge logic from step_init for a single attribute. |
| 19 | +
|
| 20 | + Returns the resolved value as a string (matching self.attributes format). |
| 21 | + """ |
| 22 | + k8s_default = KubernetesDecorator.defaults[attr] |
| 23 | + res_default = ResourcesDecorator.defaults.get(attr) |
12 | 24 |
|
| 25 | + my_val = k8s_val |
| 26 | + v = resources_val |
13 | 27 |
|
14 | | -def _merge(k, k8s_val, resources_val): |
15 | | - """Replicate the merge logic from KubernetesDecorator.step_init.""" |
16 | | - k8s_is_default = float(k8s_val or 0) == float(K8S_DEFAULTS.get(k) or 0) |
17 | | - resources_is_default = k in RESOURCES_DEFAULTS and float( |
18 | | - resources_val or 0 |
19 | | - ) == float(RESOURCES_DEFAULTS.get(k) or 0) |
| 28 | + if my_val is None and v is None: |
| 29 | + return str(my_val) |
| 30 | + |
| 31 | + k8s_is_default = float(my_val or 0) == float(k8s_default or 0) |
| 32 | + resources_is_default = attr in ResourcesDecorator.defaults and float( |
| 33 | + v or 0 |
| 34 | + ) == float(res_default or 0) |
20 | 35 |
|
21 | 36 | if k8s_is_default and not resources_is_default: |
22 | | - return str(float(resources_val or 0)) |
| 37 | + return str(float(v or 0)) |
23 | 38 | else: |
24 | | - return str(max(float(k8s_val or 0), float(resources_val or 0))) |
| 39 | + return str(max(float(my_val or 0), float(v or 0))) |
25 | 40 |
|
26 | 41 |
|
27 | | -def test_resources_lower_than_k8s_default(): |
28 | | - """@resources(memory=256) with @kubernetes default (4096) → resources wins.""" |
29 | | - result = _merge("memory", K8S_DEFAULTS["memory"], "256") |
30 | | - assert result == "256.0" |
| 42 | +def test_resources_lower_than_k8s_default_wins(): |
| 43 | + """@resources(memory=256) should override @kubernetes default (4096).""" |
| 44 | + result = _merge_resource("4096", "256") |
| 45 | + assert float(result) == 256.0 |
31 | 46 |
|
32 | 47 |
|
33 | | -def test_resources_higher_than_k8s_default(): |
34 | | - """@resources(memory=8192) with @kubernetes default (4096) → resources wins.""" |
35 | | - result = _merge("memory", K8S_DEFAULTS["memory"], "8192") |
36 | | - assert result == "8192.0" |
| 48 | +def test_resources_higher_than_k8s_default_wins(): |
| 49 | + """@resources(memory=8192) overrides @kubernetes default (4096).""" |
| 50 | + result = _merge_resource("4096", "8192") |
| 51 | + assert float(result) == 8192.0 |
37 | 52 |
|
38 | 53 |
|
39 | | -def test_both_explicit_resources_higher(): |
40 | | - """@kubernetes(memory=2048) explicit with @resources(memory=8192) → max wins.""" |
41 | | - result = _merge("memory", "2048", "8192") |
42 | | - assert result == "8192.0" |
| 54 | +def test_both_explicit_takes_max(): |
| 55 | + """@kubernetes(memory=2048) + @resources(memory=8192) → max wins.""" |
| 56 | + result = _merge_resource("2048", "8192") |
| 57 | + # k8s is not at default (2048 != 4096), so max(2048, 8192) = 8192 |
| 58 | + assert float(result) == 8192.0 |
43 | 59 |
|
44 | 60 |
|
45 | 61 | def test_both_explicit_k8s_higher(): |
46 | | - """@kubernetes(memory=8192) explicit with @resources(memory=2048) → max wins.""" |
47 | | - result = _merge("memory", "8192", "2048") |
48 | | - assert result == "8192.0" |
| 62 | + """@kubernetes(memory=8192) + @resources(memory=2048) → k8s wins.""" |
| 63 | + result = _merge_resource("8192", "2048") |
| 64 | + assert float(result) == 8192.0 |
49 | 65 |
|
50 | 66 |
|
51 | 67 | def test_both_at_default(): |
52 | | - """Both at default → stays at default (4096).""" |
53 | | - result = _merge("memory", K8S_DEFAULTS["memory"], RESOURCES_DEFAULTS["memory"]) |
54 | | - assert result == "4096.0" |
| 68 | + """Both at default → stays at default.""" |
| 69 | + k8s_default = KubernetesDecorator.defaults["memory"] |
| 70 | + res_default = ResourcesDecorator.defaults["memory"] |
| 71 | + result = _merge_resource(k8s_default, res_default) |
| 72 | + assert float(result) == float(k8s_default) |
| 73 | + |
| 74 | + |
| 75 | +def test_cpu_resources_overrides_k8s_default(): |
| 76 | + """@resources(cpu=2) should override @kubernetes default cpu (1).""" |
| 77 | + result = _merge_resource("1", "2", attr="cpu") |
| 78 | + assert float(result) == 2.0 |
0 commit comments