Skip to content

Commit 62460d2

Browse files
committed
fixup! Let the HITL review plugin address one loop pass or mapped slot
T10.7: the test that autospecced XComModel.get_many with the plugin's version flag forced to True raised TypeError on the 3.1 to 3.3 compat hosts, whose real signature has no region_id, and only checked that a keyword was present, never its value. The regional and mapped review tests already cover the region arguments with real values on 3.4 and later, and every compat run exercises the unpatched path, so the test is removed. T10.8: the regional review fixture only had two task instances in the same region, so dropping the region_id filter from _write_xcom or _is_task_completed failed nothing. It now forks the region and adds a same-task, same-index task instance in each region, with the decoy sorting first both by insert order and by the state index, so the reviewed pass is the fork one. Removing either filter now fails the test. T10.9: the docs did not say where region_id and region_index come from or that they also address mapped slots. They are the task instance's own fields from the public task instance endpoints. T10.10: the node tests under www/tests were never run by anything, and the prek hook did not trigger on .mjs edits. Add a pnpm test script, run it from the hook, and widen the hook's file pattern to include .mjs. T10.8, reworked: the forked-region fixture in TestRegionalReview put two live task instances for one task at the same region_index in two regions of one fork chain. A clear archives the original region's rows at and after the fork's resume index, so that state never exists and the test did not describe what the region_id filters in _write_xcom and _is_task_completed guard against. Restore the original fixture and cover the real case: a mapped HITL task inside a loop body, where each pass gets its own nested mapped region (parent_region_index is the pass number), so pass 0 slot 0 and pass 1 slot 0 share a map index in different regions and are both live. The new tests respond to the second pass and check the first pass keeps its pending review, and check task_completed per pass. T10.8, defensive case: a clear racing the scheduler can leave a live task instance beyond a fork's cut, which the code tolerates through DynamicRegion.is_coordinate_superseded. Cover it next to the mapped-in-loop case: a live row for the same task and region_index in the forked-from region, which sorts before the fork's row, must not receive a response meant for the fork nor change the fork's completion state. This keeps the region_id filters in _write_xcom and _is_task_completed pinned for a state that should be impossible in the database but is handled in code.
1 parent a8f25f3 commit 62460d2

5 files changed

Lines changed: 220 additions & 18 deletions

File tree

‎providers/common/ai/.pre-commit-config.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ repos:
3030
language: node
3131
files: |
3232
(?x)
33-
^src/airflow/providers/common/ai/plugins/www/.*\.(js|ts|tsx|yaml|css|json)$
33+
^src/airflow/providers/common/ai/plugins/www/.*\.(m?js|ts|tsx|yaml|css|json)$
3434
exclude: |
3535
(?x)
3636
^src/airflow/providers/common/ai/plugins/www/node-modules/.*|

‎providers/common/ai/docs/hitl_review.rst‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,10 +167,11 @@ The plugin exposes a FastAPI app at ``/hitl-review``. Base URL:
167167
- ``run_id``: Dag run ID.
168168
- ``task_id``: Task ID.
169169
- ``map_index``: Map index for mapped tasks. Use ``-1`` for non-mapped tasks or index for dynamic mapping.
170-
- ``region_id`` and ``region_index``: Select one pass of a loop. Send them together; ``region_index``
171-
without ``region_id`` is rejected. Only supported on Airflow 3.4 or later; earlier hosts return 400 when
172-
either is sent. A task inside a loop addressed without them returns 400, and a selection that matches
173-
more than one task instance returns 409.
170+
- ``region_id`` and ``region_index``: Select one loop pass or one mapped slot. Use the ``region_id`` and
171+
``region_index`` of the task instance as returned by the public task instance endpoints. Send them
172+
together; ``region_index`` without ``region_id`` is rejected. Only supported on Airflow 3.4 or later;
173+
earlier hosts return 400 when either is sent. A task inside a loop addressed without them returns 400,
174+
and a selection that matches more than one task instance returns 409.
174175

175176
Endpoints
176177
^^^^^^^^^

‎providers/common/ai/src/airflow/providers/common/ai/plugins/www/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
"build:types": "tsc --p tsconfig.lib.json",
2626
"build:lib": "vite build",
2727
"lint": "tsc --p tsconfig.app.json --noEmit",
28+
"test": "node --test --experimental-strip-types \"tests/**/*.test.*\"",
2829
"preview": "vite preview"
2930
},
3031
"dependencies": {

‎providers/common/ai/tests/unit/common/ai/plugins/test_hitl_review.py‎

Lines changed: 212 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030

3131
import time_machine
3232
from fastapi.testclient import TestClient
33-
from sqlalchemy import false, select
33+
from sqlalchemy import select
3434

3535
from airflow.api_fastapi.app import create_app, purge_cached_app
3636
from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser
@@ -60,6 +60,7 @@
6060
from airflow.utils.types import DagRunType
6161

6262
if AIRFLOW_V_3_4_PLUS:
63+
from airflow.models.dynamic_region import DynamicRegion
6364
from airflow.models.taskinstance import TaskInstance
6465
from airflow.sdk import task_group
6566
from airflow.sdk.definitions._internal.loop import create_loop
@@ -565,18 +566,6 @@ def test_read_xcom_key_value_combinations(self, session, dag_maker, key, value,
565566
assert result == expected
566567
_clear_db()
567568

568-
@pytest.mark.parametrize(("is_v3_4_plus", "expect_region"), [(True, True), (False, False)])
569-
@mock.patch("airflow.providers.common.ai.plugins.hitl_review.XComModel.get_many", autospec=True)
570-
def test_region_is_only_passed_to_airflow_that_supports_it(
571-
self, mock_get_many, session, is_v3_4_plus, expect_region
572-
):
573-
mock_get_many.return_value = select(1).where(false())
574-
575-
with mock.patch("airflow.providers.common.ai.plugins.hitl_review.AIRFLOW_V_3_4_PLUS", is_v3_4_plus):
576-
_read_xcom(session, region_id=NO_REGION, dag_id="d", run_id="r", task_id="t", key="k")
577-
578-
assert ("region_id" in mock_get_many.call_args.kwargs) is expect_region
579-
580569

581570
class TestWriteXcom:
582571
"""Test _write_xcom."""
@@ -1173,6 +1162,114 @@ def test_loop_scope_is_required(self, regional_review, test_client):
11731162
assert "loop producer requires" in response.json()["detail"]
11741163

11751164

1165+
@pytest.mark.skipif(not AIRFLOW_V_3_4_PLUS, reason="Regional task identity requires Airflow 3.4+")
1166+
class TestForkedRegionReview:
1167+
# A live row beyond a fork's cut should be impossible; this is a defensive test for a clear racing the scheduler.
1168+
@pytest.fixture
1169+
def forked_passes(self, dag_maker, session):
1170+
_clear_db()
1171+
1172+
@task_group
1173+
def body():
1174+
EmptyOperator(task_id="review")
1175+
1176+
with dag_maker(TEST_DAG_ID, serialized=True):
1177+
create_loop(body, max_iterations=2)
1178+
dr = dag_maker.create_dagrun(run_id=TEST_RUN_ID)
1179+
first = next(ti for ti in dr.task_instances if ti.task_id == "body.review")
1180+
region = session.get(DynamicRegion, first.region_id)
1181+
fork = DynamicRegion(
1182+
dag_id=region.dag_id,
1183+
run_id=region.run_id,
1184+
node_id=region.node_id,
1185+
forked_from_region_id=region.id,
1186+
)
1187+
session.add(fork)
1188+
session.flush()
1189+
task = dag_maker.serialized_dag.get_task(first.task_id)
1190+
beyond_cut = TaskInstance(
1191+
task=task,
1192+
run_id=dr.run_id,
1193+
dag_version_id=first.dag_version_id,
1194+
region_id=region.id,
1195+
region_index=1,
1196+
state="failed",
1197+
)
1198+
forked = TaskInstance(
1199+
task=task,
1200+
run_id=dr.run_id,
1201+
dag_version_id=first.dag_version_id,
1202+
region_id=fork.id,
1203+
region_index=1,
1204+
state="running",
1205+
)
1206+
session.add_all([beyond_cut, forked])
1207+
for ti, output in ((beyond_cut, "beyond cut"), (forked, "forked")):
1208+
values = {
1209+
XCOM_AGENT_SESSION: AgentSessionData(
1210+
status=SessionStatus.PENDING_REVIEW,
1211+
iteration=1,
1212+
max_iterations=5,
1213+
current_output=output,
1214+
).model_dump(mode="json"),
1215+
f"{XCOM_AGENT_OUTPUT_PREFIX}1": output,
1216+
}
1217+
session.flush()
1218+
for key, value in values.items():
1219+
XComModel.set_for_attempt(
1220+
task_instance_id=ti.id, key=key, value=value, serialize=False, session=session
1221+
)
1222+
session.commit()
1223+
yield beyond_cut, forked
1224+
_clear_db()
1225+
1226+
@staticmethod
1227+
def _params(ti):
1228+
return {
1229+
"dag_id": ti.dag_id,
1230+
"run_id": ti.run_id,
1231+
"task_id": ti.task_id,
1232+
"region_id": str(ti.region_id),
1233+
"region_index": ti.region_index,
1234+
}
1235+
1236+
@pytest.mark.parametrize(
1237+
("action", "expected_status"),
1238+
[
1239+
("feedback", "changes_requested"),
1240+
("approve", "approved"),
1241+
("reject", "rejected"),
1242+
],
1243+
)
1244+
def test_response_ignores_live_row_left_beyond_a_fork_cut(
1245+
self, forked_passes, test_client, action, expected_status
1246+
):
1247+
beyond_cut, forked = forked_passes
1248+
1249+
response = test_client.post(
1250+
f"/hitl-review/sessions/{action}", params=self._params(forked), json={"feedback": "revise"}
1251+
)
1252+
1253+
assert response.status_code == 200, response.text
1254+
assert response.json()["status"] == expected_status
1255+
assert response.json()["current_output"] == "forked"
1256+
other = test_client.get("/hitl-review/sessions/find", params=self._params(beyond_cut))
1257+
assert other.status_code == 200, other.text
1258+
assert other.json()["status"] == "pending_review"
1259+
assert other.json()["current_output"] == "beyond cut"
1260+
1261+
@pytest.mark.parametrize(("pass_number", "expected_completed"), [(0, True), (1, False)])
1262+
def test_completion_ignores_live_row_left_beyond_a_fork_cut(
1263+
self, forked_passes, test_client, pass_number, expected_completed
1264+
):
1265+
response = test_client.get(
1266+
"/hitl-review/sessions/find", params=self._params(forked_passes[pass_number])
1267+
)
1268+
1269+
assert response.status_code == 200, response.text
1270+
assert response.json()["task_completed"] is expected_completed
1271+
1272+
11761273
@pytest.mark.skipif(not AIRFLOW_V_3_4_PLUS, reason="Regional task identity requires Airflow 3.4+")
11771274
class TestMappedReview:
11781275
@pytest.fixture
@@ -1244,6 +1341,108 @@ def test_explicit_sentinel_region_does_not_read_the_node_region_session(self, ma
12441341
assert response.status_code == 404
12451342

12461343

1344+
@pytest.mark.skipif(not AIRFLOW_V_3_4_PLUS, reason="Regional task identity requires Airflow 3.4+")
1345+
class TestMappedReviewInLoop:
1346+
@pytest.fixture
1347+
def loop_passes(self, dag_maker, session):
1348+
_clear_db()
1349+
1350+
@task_group
1351+
def body():
1352+
PythonOperator.partial(task_id="mapped", python_callable=str).expand(op_args=[[1]])
1353+
1354+
with dag_maker(TEST_DAG_ID, serialized=True):
1355+
create_loop(body, max_iterations=2)
1356+
dr = dag_maker.create_dagrun(run_id=TEST_RUN_ID)
1357+
first_pass = next(ti for ti in dr.task_instances if ti.task_id == "body.mapped")
1358+
first_region = session.get(DynamicRegion, first_pass.region_id)
1359+
second_region = DynamicRegion.get_or_create(
1360+
dag_id=first_region.dag_id,
1361+
run_id=first_region.run_id,
1362+
node_id=first_region.node_id,
1363+
parent_region_id=first_region.parent_region_id,
1364+
parent_region_index=1,
1365+
session=session,
1366+
)
1367+
session.flush()
1368+
second_pass = TaskInstance(
1369+
task=dag_maker.serialized_dag.get_task(first_pass.task_id),
1370+
run_id=dr.run_id,
1371+
dag_version_id=first_pass.dag_version_id,
1372+
region_id=second_region.id,
1373+
region_index=first_pass.region_index,
1374+
state="running",
1375+
)
1376+
session.add(second_pass)
1377+
first_pass.state = "failed"
1378+
for ti, output in ((first_pass, "first pass"), (second_pass, "second pass")):
1379+
values = {
1380+
XCOM_AGENT_SESSION: AgentSessionData(
1381+
status=SessionStatus.PENDING_REVIEW,
1382+
iteration=1,
1383+
max_iterations=5,
1384+
current_output=output,
1385+
).model_dump(mode="json"),
1386+
f"{XCOM_AGENT_OUTPUT_PREFIX}1": output,
1387+
}
1388+
session.flush()
1389+
for key, value in values.items():
1390+
XComModel.set_for_attempt(
1391+
task_instance_id=ti.id, key=key, value=value, serialize=False, session=session
1392+
)
1393+
session.commit()
1394+
yield first_pass, second_pass
1395+
_clear_db()
1396+
1397+
@staticmethod
1398+
def _params(ti):
1399+
return {
1400+
"dag_id": ti.dag_id,
1401+
"run_id": ti.run_id,
1402+
"task_id": ti.task_id,
1403+
"region_id": str(ti.region_id),
1404+
"region_index": ti.region_index,
1405+
}
1406+
1407+
@pytest.mark.parametrize(
1408+
("action", "expected_status"),
1409+
[
1410+
("feedback", "changes_requested"),
1411+
("approve", "approved"),
1412+
("reject", "rejected"),
1413+
],
1414+
)
1415+
def test_response_for_one_pass_leaves_the_same_map_index_in_the_other_pass(
1416+
self, loop_passes, test_client, session, action, expected_status
1417+
):
1418+
first_pass, second_pass = loop_passes
1419+
assert first_pass.region_id != second_pass.region_id
1420+
assert first_pass.region_index == second_pass.region_index
1421+
1422+
response = test_client.post(
1423+
f"/hitl-review/sessions/{action}", params=self._params(second_pass), json={"feedback": "revise"}
1424+
)
1425+
1426+
assert response.status_code == 200, response.text
1427+
assert response.json()["status"] == expected_status
1428+
assert response.json()["current_output"] == "second pass"
1429+
other = test_client.get("/hitl-review/sessions/find", params=self._params(first_pass))
1430+
assert other.status_code == 200, other.text
1431+
assert other.json()["status"] == "pending_review"
1432+
assert other.json()["current_output"] == "first pass"
1433+
1434+
@pytest.mark.parametrize(("pass_number", "expected_completed"), [(0, True), (1, False)])
1435+
def test_task_completion_is_reported_for_the_selected_pass(
1436+
self, loop_passes, test_client, pass_number, expected_completed
1437+
):
1438+
response = test_client.get(
1439+
"/hitl-review/sessions/find", params=self._params(loop_passes[pass_number])
1440+
)
1441+
1442+
assert response.status_code == 200, response.text
1443+
assert response.json()["task_completed"] is expected_completed
1444+
1445+
12471446
class TestFindSessionEndpoint:
12481447
"""Test the /sessions/find endpoint."""
12491448

‎scripts/ci/prek/ts_compile_lint_common_ai.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
print("All TypeScript files:", all_ts_files)
6161

6262
run_command(["pnpm", "install", "--frozen-lockfile", "--config.confirmModulesPurge=false"], cwd=dir)
63+
run_command(["pnpm", "test"], cwd=dir)
6364
if any("/openapi/" in file for file in original_files):
6465
run_command(["pnpm", "codegen"], cwd=dir)
6566
if all_non_yaml_files:

0 commit comments

Comments
 (0)