|
| 1 | +""" |
| 2 | +Action-name stability — remaining validation scope (ENG26-831). |
| 3 | +
|
| 4 | +Every test here asserts the behavior recovery REQUIRES: running the same workflow twice |
| 5 | +must produce the identical set of action names. Tests that FAIL confirm a live |
| 6 | +instability that breaks recovery matching; tests that pass close out their row of the |
| 7 | +stability matrix. |
| 8 | +
|
| 9 | +Confirmed instabilities are marked ``xfail(strict=True)`` so the suite stays green until |
| 10 | +each fix lands — the strict marker then forces its removal: |
| 11 | +
|
| 12 | +* ``TestGroupSequencerInteraction`` — the sequencer call key (task identity + inputs |
| 13 | + hash) does not include the group, but the group IS folded into the action name. Two |
| 14 | + byte-identical calls made from different groups share one counter, so which group gets |
| 15 | + seq 1 vs 2 depends on event-loop scheduling order — and because the group differs, the |
| 16 | + resulting names are NOT interchangeable. The whole name set flips run-to-run. |
| 17 | +* ``TestUnorderedInputs::test_untyped_dict_*`` — untyped ``dict`` inputs serialize via |
| 18 | + msgpack, which preserves insertion order; semantically-equal dicts built in different |
| 19 | + key orders produce different input hashes. |
| 20 | +* ``TestUnorderedInputs::test_set_inputs_stable_across_processes`` — ``Set[str]`` falls |
| 21 | + back to pickle, which serializes in set iteration order; iteration order of str sets |
| 22 | + depends on PYTHONHASHSEED, so the hash differs across interpreter processes (i.e. |
| 23 | + across any two real runs). |
| 24 | +""" |
| 25 | + |
| 26 | +from __future__ import annotations |
| 27 | + |
| 28 | +import os |
| 29 | +import subprocess |
| 30 | +import sys |
| 31 | +import textwrap |
| 32 | +from typing import Dict, List |
| 33 | + |
| 34 | +import pytest |
| 35 | +from flyteidl2.core import literals_pb2, types_pb2 |
| 36 | + |
| 37 | +from flyte._internal.controllers import TaskCallSequencer |
| 38 | +from flyte._internal.runtime import convert |
| 39 | +from flyte.models import ActionID, GroupData, RawDataPath, TaskContext |
| 40 | +from flyte.report import Report |
| 41 | +from flyte.types import TypeEngine |
| 42 | + |
| 43 | +TASK_IDENTITY = "task-identity-hash" |
| 44 | +INPUTS_HASH = "inputs-hash" |
| 45 | + |
| 46 | + |
| 47 | +def _make_tctx() -> TaskContext: |
| 48 | + return TaskContext( |
| 49 | + action=ActionID(name="parent", run_name="run1", project="p", domain="d"), |
| 50 | + run_base_dir="s3://bucket/metadata/p/d/run1", |
| 51 | + version="v1", |
| 52 | + raw_data_path=RawDataPath(path="s3://bucket/raw/p/d/run1"), |
| 53 | + output_path="s3://bucket/output/p/d/run1", |
| 54 | + report=Report(name="test"), |
| 55 | + ) |
| 56 | + |
| 57 | + |
| 58 | +def _submit_name( |
| 59 | + sequencer: TaskCallSequencer, |
| 60 | + tctx: TaskContext, |
| 61 | + task_identity: str, |
| 62 | + inputs_hash: str, |
| 63 | + group: str | None = None, |
| 64 | +) -> str: |
| 65 | + """Replica of the remote controller's naming path (_controller.py::_submit): |
| 66 | + call_key = task identity + inputs hash (group NOT included), name folds in group.""" |
| 67 | + seq = sequencer.next_seq(f"{task_identity}:{inputs_hash}", tctx.action.name) |
| 68 | + call_tctx = tctx.replace(group_data=GroupData(group)) if group else tctx |
| 69 | + sub_id, _ = convert.generate_sub_action_id_and_output_path(call_tctx, task_identity, inputs_hash, seq) |
| 70 | + return sub_id.name |
| 71 | + |
| 72 | + |
| 73 | +def _simulate_run(calls: list[tuple[str, str, str | None]]) -> set[str]: |
| 74 | + """Simulate one run: submit *calls* (task_identity, inputs_hash, group) in arrival |
| 75 | + order and return the resulting action-name set.""" |
| 76 | + sequencer = TaskCallSequencer() |
| 77 | + tctx = _make_tctx() |
| 78 | + return {_submit_name(sequencer, tctx, ti, ih, g) for ti, ih, g in calls} |
| 79 | + |
| 80 | + |
| 81 | +class TestGroupSequencerInteraction: |
| 82 | + """Groups are folded into the name but not into the sequencer key, so identical |
| 83 | + calls made from different groups race for sequence numbers.""" |
| 84 | + |
| 85 | + @pytest.mark.xfail( |
| 86 | + strict=True, reason="ENG26-831: group is folded into the name but not into the sequencer call key" |
| 87 | + ) |
| 88 | + def test_same_call_in_two_groups_scheduling_order_insensitive(self): |
| 89 | + """Same task + same inputs invoked from two different groups (independent async |
| 90 | + branches): the name set must not depend on which branch reaches the controller |
| 91 | + first.""" |
| 92 | + run1 = _simulate_run([(TASK_IDENTITY, INPUTS_HASH, "group_a"), (TASK_IDENTITY, INPUTS_HASH, "group_b")]) |
| 93 | + run2 = _simulate_run([(TASK_IDENTITY, INPUTS_HASH, "group_b"), (TASK_IDENTITY, INPUTS_HASH, "group_a")]) |
| 94 | + assert run1 == run2 |
| 95 | + |
| 96 | + @pytest.mark.xfail( |
| 97 | + strict=True, reason="ENG26-831: group is folded into the name but not into the sequencer call key" |
| 98 | + ) |
| 99 | + def test_grouped_and_ungrouped_call_scheduling_order_insensitive(self): |
| 100 | + """Same task + same inputs invoked once inside a group and once outside.""" |
| 101 | + run1 = _simulate_run([(TASK_IDENTITY, INPUTS_HASH, "group_a"), (TASK_IDENTITY, INPUTS_HASH, None)]) |
| 102 | + run2 = _simulate_run([(TASK_IDENTITY, INPUTS_HASH, None), (TASK_IDENTITY, INPUTS_HASH, "group_a")]) |
| 103 | + assert run1 == run2 |
| 104 | + |
| 105 | + @pytest.mark.xfail( |
| 106 | + strict=True, reason="ENG26-831: group is folded into the name but not into the sequencer call key" |
| 107 | + ) |
| 108 | + def test_two_named_maps_over_same_items_scheduling_order_insensitive(self): |
| 109 | + """Two concurrent flyte.map calls (distinct group_name) mapping the same task |
| 110 | + over the same items: per-shard names must not depend on how the two maps |
| 111 | + interleave.""" |
| 112 | + items = ["item_hash_1", "item_hash_1"] # duplicate items across the two maps |
| 113 | + map_a = [(TASK_IDENTITY, ih, "t_map_a") for ih in items] |
| 114 | + map_b = [(TASK_IDENTITY, ih, "t_map_b") for ih in items] |
| 115 | + # run 1: map_a fully first; run 2: interleaved starting with map_b |
| 116 | + run1 = _simulate_run(map_a + map_b) |
| 117 | + run2 = _simulate_run([map_b[0], map_a[0], map_b[1], map_a[1]]) |
| 118 | + assert run1 == run2 |
| 119 | + |
| 120 | + |
| 121 | +class TestMapFanOut: |
| 122 | + """Fan-out within a single map/group (single counter per (identity, inputs)).""" |
| 123 | + |
| 124 | + def test_distinct_items_stable_regardless_of_completion_order(self): |
| 125 | + """Distinct items never share a counter → order-insensitive.""" |
| 126 | + items = ["item_hash_1", "item_hash_2", "item_hash_3"] |
| 127 | + run1 = _simulate_run([(TASK_IDENTITY, ih, "t_map") for ih in items]) |
| 128 | + run2 = _simulate_run([(TASK_IDENTITY, ih, "t_map") for ih in reversed(items)]) |
| 129 | + assert run1 == run2 |
| 130 | + |
| 131 | + def test_duplicate_items_within_one_map_interchangeable(self): |
| 132 | + """Duplicate items in one map share a counter, but within a single group the |
| 133 | + resulting names are byte-identical calls — the name SET is stable.""" |
| 134 | + calls = [(TASK_IDENTITY, "item_hash_1", "t_map")] * 3 + [(TASK_IDENTITY, "item_hash_2", "t_map")] |
| 135 | + run1 = _simulate_run(calls) |
| 136 | + run2 = _simulate_run(list(reversed(calls))) |
| 137 | + assert run1 == run2 |
| 138 | + |
| 139 | + |
| 140 | +class TestTaskCallSequencer: |
| 141 | + """Direct unit tests for the sequencer (previously untested).""" |
| 142 | + |
| 143 | + def test_different_call_keys_do_not_share_counters(self): |
| 144 | + s = TaskCallSequencer() |
| 145 | + assert s.next_seq("id1:inputs_a", "parent") == 1 |
| 146 | + assert s.next_seq("id1:inputs_b", "parent") == 1 |
| 147 | + assert s.next_seq("id2:inputs_a", "parent") == 1 |
| 148 | + |
| 149 | + def test_same_call_key_increments(self): |
| 150 | + s = TaskCallSequencer() |
| 151 | + assert s.next_seq("id1:inputs_a", "parent") == 1 |
| 152 | + assert s.next_seq("id1:inputs_a", "parent") == 2 |
| 153 | + |
| 154 | + def test_counters_scoped_per_parent_action(self): |
| 155 | + s = TaskCallSequencer() |
| 156 | + assert s.next_seq("id1:inputs_a", "parent1") == 1 |
| 157 | + assert s.next_seq("id1:inputs_a", "parent2") == 1 |
| 158 | + |
| 159 | + def test_clear_resets_parent(self): |
| 160 | + s = TaskCallSequencer() |
| 161 | + s.next_seq("id1:inputs_a", "parent1") |
| 162 | + s.clear("parent1") |
| 163 | + assert s.next_seq("id1:inputs_a", "parent1") == 1 |
| 164 | + |
| 165 | + |
| 166 | +class TestStructuralChanges: |
| 167 | + """Loops / conditionals: since counters are keyed by (identity + inputs), only |
| 168 | + byte-identical repeated calls share a counter — inserting or removing a call with |
| 169 | + different inputs must not shift anyone else's name.""" |
| 170 | + |
| 171 | + def test_appending_an_identical_call_preserves_existing_names(self): |
| 172 | + """Loop grows from 2 to 3 iterations over the same inputs: the first two names |
| 173 | + must be unchanged (recovery reuses them; only the new call runs).""" |
| 174 | + seq_run1 = TaskCallSequencer() |
| 175 | + seq_run2 = TaskCallSequencer() |
| 176 | + tctx = _make_tctx() |
| 177 | + run1 = [_submit_name(seq_run1, tctx, TASK_IDENTITY, INPUTS_HASH) for _ in range(2)] |
| 178 | + run2 = [_submit_name(seq_run2, tctx, TASK_IDENTITY, INPUTS_HASH) for _ in range(3)] |
| 179 | + assert run2[:2] == run1 |
| 180 | + |
| 181 | + def test_inserting_a_different_call_does_not_shift_others(self): |
| 182 | + """A new conditional branch adds a call with different inputs between two |
| 183 | + existing identical calls — the existing calls' names must not change.""" |
| 184 | + run1 = _simulate_run([(TASK_IDENTITY, INPUTS_HASH, None), (TASK_IDENTITY, INPUTS_HASH, None)]) |
| 185 | + run2_calls = [ |
| 186 | + (TASK_IDENTITY, INPUTS_HASH, None), |
| 187 | + (TASK_IDENTITY, "new_branch_inputs", None), |
| 188 | + (TASK_IDENTITY, INPUTS_HASH, None), |
| 189 | + ] |
| 190 | + run2 = _simulate_run(run2_calls) |
| 191 | + assert run1 <= run2 |
| 192 | + |
| 193 | + |
| 194 | +class TestUnorderedInputs: |
| 195 | + """Input hashing must be insensitive to semantically-irrelevant ordering.""" |
| 196 | + |
| 197 | + @pytest.mark.xfail(strict=True, reason="ENG26-831: untyped dicts serialize via insertion-ordered msgpack") |
| 198 | + @pytest.mark.asyncio |
| 199 | + async def test_untyped_dict_insertion_order_stable(self): |
| 200 | + """Untyped dicts serialize via msgpack (insertion-ordered): two equal dicts |
| 201 | + built in different key orders must still hash identically.""" |
| 202 | + lt = TypeEngine.to_literal_type(dict) |
| 203 | + lit_ab = await TypeEngine.to_literal({"a": 1, "b": 2}, dict, lt) |
| 204 | + lit_ba = await TypeEngine.to_literal({"b": 2, "a": 1}, dict, lt) |
| 205 | + assert convert.generate_inputs_repr_for_literal(lit_ab) == convert.generate_inputs_repr_for_literal(lit_ba) |
| 206 | + |
| 207 | + @pytest.mark.asyncio |
| 208 | + async def test_typed_dict_insertion_order_stable(self): |
| 209 | + """Dict[str, T] becomes a map literal whose keys are sorted at hash time.""" |
| 210 | + t = Dict[str, int] |
| 211 | + lt = TypeEngine.to_literal_type(t) |
| 212 | + lit_ab = await TypeEngine.to_literal({"a": 1, "b": 2}, t, lt) |
| 213 | + lit_ba = await TypeEngine.to_literal({"b": 2, "a": 1}, t, lt) |
| 214 | + assert convert.generate_inputs_repr_for_literal(lit_ab) == convert.generate_inputs_repr_for_literal(lit_ba) |
| 215 | + |
| 216 | + @pytest.mark.xfail(strict=True, reason="ENG26-831: set inputs pickle in PYTHONHASHSEED-dependent iteration order") |
| 217 | + def test_set_inputs_stable_across_processes(self): |
| 218 | + """Set[str] inputs fall back to pickle in set iteration order, which depends on |
| 219 | + PYTHONHASHSEED — two runs (two interpreter processes) of the same workflow must |
| 220 | + still produce the same inputs hash.""" |
| 221 | + script = textwrap.dedent( |
| 222 | + """ |
| 223 | + import asyncio, hashlib |
| 224 | + from typing import Set |
| 225 | + from flyte._internal.runtime import convert |
| 226 | + from flyte.types import TypeEngine |
| 227 | +
|
| 228 | + async def main(): |
| 229 | + st = Set[str] |
| 230 | + lt = TypeEngine.to_literal_type(st) |
| 231 | + lit = await TypeEngine.to_literal( |
| 232 | + {"alpha", "bravo", "charlie", "delta", "echo"}, st, lt |
| 233 | + ) |
| 234 | + print(hashlib.md5(convert.generate_inputs_repr_for_literal(lit)).hexdigest()) |
| 235 | +
|
| 236 | + asyncio.run(main()) |
| 237 | + """ |
| 238 | + ) |
| 239 | + |
| 240 | + def run_with_seed(seed: str) -> str: |
| 241 | + env = {**os.environ, "PYTHONHASHSEED": seed} |
| 242 | + out = subprocess.run([sys.executable, "-c", script], env=env, capture_output=True, text=True, check=True) |
| 243 | + return out.stdout.strip().splitlines()[-1] |
| 244 | + |
| 245 | + assert run_with_seed("1") == run_with_seed("42") |
| 246 | + |
| 247 | + @pytest.mark.asyncio |
| 248 | + async def test_list_order_is_significant(self): |
| 249 | + """Sanity check (not an instability): list order is data — different orders are |
| 250 | + different inputs and must hash differently.""" |
| 251 | + t = List[int] |
| 252 | + lt = TypeEngine.to_literal_type(t) |
| 253 | + lit_12 = await TypeEngine.to_literal([1, 2], t, lt) |
| 254 | + lit_21 = await TypeEngine.to_literal([2, 1], t, lt) |
| 255 | + assert convert.generate_inputs_repr_for_literal(lit_12) != convert.generate_inputs_repr_for_literal(lit_21) |
| 256 | + |
| 257 | + |
| 258 | +def _blob_literal(uri: str, hash_value: str = "") -> literals_pb2.Literal: |
| 259 | + lit = literals_pb2.Literal( |
| 260 | + scalar=literals_pb2.Scalar( |
| 261 | + blob=literals_pb2.Blob( |
| 262 | + metadata=literals_pb2.BlobMetadata( |
| 263 | + type=types_pb2.BlobType(format="", dimensionality=types_pb2.BlobType.SINGLE) |
| 264 | + ), |
| 265 | + uri=uri, |
| 266 | + ) |
| 267 | + ) |
| 268 | + ) |
| 269 | + if hash_value: |
| 270 | + lit.hash = hash_value |
| 271 | + return lit |
| 272 | + |
| 273 | + |
| 274 | +class TestOffloadedLiteralUris: |
| 275 | + """File/Dir/DataFrame literals embed URIs containing the source run name; recovery |
| 276 | + correctness depends on how those URIs feed the input hash.""" |
| 277 | + |
| 278 | + def test_precomputed_hash_wins_over_uri(self): |
| 279 | + """A literal carrying a content hash must hash the same regardless of which |
| 280 | + run's URI it points at — recovered upstream output (old URI) then matches.""" |
| 281 | + lit_run1 = _blob_literal("s3://bucket/outputs/run1/file", hash_value="content-hash") |
| 282 | + lit_run2 = _blob_literal("s3://bucket/outputs/run2/file", hash_value="content-hash") |
| 283 | + assert convert.generate_inputs_repr_for_literal(lit_run1) == convert.generate_inputs_repr_for_literal(lit_run2) |
| 284 | + |
| 285 | + def test_uri_change_without_hash_changes_input_hash(self): |
| 286 | + """Without a content hash the URI is the identity: a re-run upstream (new URI) |
| 287 | + must invalidate downstream (consistent rerun cascade, documented in ENG26-1042).""" |
| 288 | + lit_run1 = _blob_literal("s3://bucket/outputs/run1/file") |
| 289 | + lit_run2 = _blob_literal("s3://bucket/outputs/run2/file") |
| 290 | + assert convert.generate_inputs_repr_for_literal(lit_run1) != convert.generate_inputs_repr_for_literal(lit_run2) |
0 commit comments