kaxil commented on code in PR #74671:
URL: https://github.com/apache/airflow/pull/74671#discussion_r4265761566
##########
airflow-core/src/airflow/ui/src/utils/executionParams.ts:
##########
@@ -36,3 +39,21 @@ export const stripExecutionParams = (searchParams: string |
URLSearchParams): UR
return stripped;
};
+
+/**
+ * Point a link at one execution. A task inside a loop has a row per
iteration, so a link with
+ * no coordinate is ambiguous and the API rejects it; the newest iteration is
the sensible
+ * landing place from a view that shows the task once.
+ */
+export const setExecutionCoordinates = (
Review Comment:
Now that `stripExecutionParams` keeps `iteration` and `loop_id`, a link
built here keeps the old pass label but moves `region_id`/`region_index` to the
newest pass. `selectActivePass` prefers `iteration` over `region_index`, so if
I pick pass 0 in the breadcrumb and then click another task in the grid, graph
or task names, the page loads the newest pass's row while the breadcrumb,
switcher and Gantt outline still say pass 0. Clear and Mark then act on a row
the label doesn't name. Should setting coordinates drop `iteration`/`loop_id`,
or should the TI pages take the pass from the loaded TI's `loop_iteration`?
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/Bar.tsx:
##########
@@ -21,6 +21,7 @@ import { useParams, useSearchParams } from "react-router-dom";
import { RunTypeIcon } from "src/components/RunTypeIcon";
+import { clearCoordinates } from "src/constants/regions";
Review Comment:
`src/constants/regions` doesn't exist on this branch and `clearCoordinates`
isn't used in this file, so this is what's failing `ts-compile-lint-ui` in
Static checks. `GridButton.tsx` line 29 has the same import.
##########
airflow-core/src/airflow/ui/src/components/Clear/TaskInstance/ClearTaskInstanceDialog.tsx:
##########
@@ -364,7 +364,9 @@ const ClearTaskInstanceDialog = (props: Props) => {
},
});
}
- onCloseDialog();
+ // Closing is left to the mutation's success path. Closing here unmounted
the dialog before
Review Comment:
Without the close, the gated path reopens the main dialog mid-clear. While
the confirmation is up `isDialogOpen = openDialog && !open` hides the Modal,
then the confirmation calls `onConfirm()` and `onClose()`, so the Modal comes
back while the request is in flight. With `unmountOnExit` defaulting to true it
remounts, and `SegmentedControl` pushes its defaults back, so on failure the
user is left in a dialog with silently reset options. I traced this rather than
ran it. The comment's cause also looks off: the toast and invalidation are
`useMutation` hook callbacks, which TanStack runs even after the observer
unmounts.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##########
@@ -442,21 +442,43 @@ def _build_ti_summaries(
if isinstance(group, SerializedLoopTaskGroup)
}
regions = DynamicRegion.load_for_run(dag_id, run_id, session=session) if
loop_ids else {}
- for ti in chain([first], rows):
+ positions_by_ti = [
Review Comment:
This materializes the whole `yield_per=1000` stream into a list on every
`ti_summaries` request, even for a Dag with no loops. Could it only build the
list when `loop_ids` is non-empty?
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_instances.py:
##########
@@ -681,9 +693,17 @@ def get_task_instances(
if region_index is not None and region_id is None:
raise HTTPException(status.HTTP_400_BAD_REQUEST, "region_index
requires region_id")
if region_id is not None:
- query = query.where(TI.region_id == region_id)
- if region_index is not None:
- query = query.where(TI.region_index == region_index)
+ if dag_id == "~" or dag_run_id == "~":
+ raise HTTPException(status.HTTP_400_BAD_REQUEST, "region_id
requires a specific Dag run")
+ query = query.where(
+ region_scope_filter(
Review Comment:
This widens the public `region_id` filter from an exact region to the fork
family plus nested regions, and newly rejects `~`. Since `loop_id`/`iteration`
already give the logical pass scope, would it be simpler to keep `region_id`
exact and have the UI scope passes through `loop_id`/`iteration`? Not a compat
concern since the filter is only in 3.4.0b1, more about keeping one meaning per
filter.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstances/TaskInstances.tsx:
##########
@@ -294,16 +303,10 @@ const taskInstanceColumns = ({
},
];
-type TaskInstancesProps = {
- readonly extraFilter?: ReactNode;
- /** Selected iteration of the loop group; undefined holds the list until the
default is known. */
- readonly iteration?: string;
- readonly loopGroupId?: string;
-};
-
-export const TaskInstances = ({ extraFilter, iteration, loopGroupId }:
TaskInstancesProps = {}) => {
+export const TaskInstances = () => {
const { i18n, t: translate } = useTranslation();
const { dagId, groupId, runId, taskId } = useParams();
+ const declaredLoops = useLoopGroupIds();
Review Comment:
On the global Task Instances page there's no Dag, and `useGridStructure` has
no `enabled` guard, so this requests the grid structure with an empty Dag id.
`enabled: Boolean(dagId)` would skip it.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##########
@@ -442,21 +442,43 @@ def _build_ti_summaries(
if isinstance(group, SerializedLoopTaskGroup)
}
regions = DynamicRegion.load_for_run(dag_id, run_id, session=session) if
loop_ids else {}
- for ti in chain([first], rows):
+ positions_by_ti = [
+ (
+ ti,
+ {
+ loop_id: position
+ for loop_id in loop_ids
+ if (position := loop_position(regions, ti.region_id,
ti.region_index, loop_id)) is not None
+ },
+ )
+ for ti in chain([first], rows)
+ ]
+ # A looped task has one row per iteration. Counting them all would report
the work done
+ # across the whole loop rather than the state the task is in, so the state
counts come from
+ # the newest iteration of each invocation. Every iteration still feeds
loop_iterations,
+ # which is what reports how many ran.
+ latest_iteration: dict[tuple[str, str, UUID], int] = {}
+ for ti, positions in positions_by_ti:
+ for loop_id, (family_id, iteration) in positions.items():
+ key = (ti.task_id, loop_id, family_id)
+ if iteration > latest_iteration.get(key, -1):
+ latest_iteration[key] = iteration
+ for ti, positions in positions_by_ti:
summary = ti_details.get(ti.task_id)
if summary is None:
summary = ti_details[ti.task_id] = GridNodeAgg()
- loop_positions = {}
- for loop_id in loop_ids:
- if position := loop_position(regions, ti.region_id,
ti.region_index, loop_id):
- loop_positions[loop_id] = position
summary.add_ti(
state=ti.state,
start_date=ti.start_date,
end_date=ti.end_date,
dag_version_number=getattr(ti, "version_number", None),
has_note=bool(getattr(ti, "has_note", False)),
- loop_positions=loop_positions,
+ loop_positions=positions,
+ region=(ti.region_id, ti.region_index) if positions else None,
Review Comment:
For a mapped task inside a loop, `(ti.region_id, ti.region_index)` is the
expansion region and map slot, and whichever slot is counted last wins. Every
UI reader treats `region_index` as the pass (`selectActivePass`, the
breadcrumb, the Gantt, the mapped header), so they show a map index as the
iteration, and when that number matches no pass `DagBreadcrumb` hides the
iteration crumb and switcher entirely. Could the summary carry the enclosing
loop coordinate (family, pass) separately from the row coordinate?
##########
airflow-core/src/airflow/ui/src/layouts/Details/IterationSwitcherButton.tsx:
##########
@@ -0,0 +1,120 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { type ReactNode, useState } from "react";
+
+import { Button, VStack } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { useLocation, useSearchParams } from "react-router-dom";
+
+import { CrumbSwitcher, type CrumbShape } from "src/components/Breadcrumb";
+import { LoopOptionLabel } from "src/components/LoopOptionLabel";
+
+import { SearchParamsKeys } from "src/constants/searchParams";
+import type { LoopOption } from "src/queries/useLoopFilterOptions";
+
+type Props = {
+ readonly children: ReactNode;
+ /** Offer the whole loop as a choice. A group can show every pass at once;
one row cannot. */
+ readonly includeWholeLoop?: boolean;
+ readonly options: Array<LoopOption>;
+ readonly shape: CrumbShape;
+};
+
+/**
+ * The loop iteration level of the breadcrumb, with the loop's passes behind
its chevron.
+ *
+ * A pass is part of where you are rather than a filter you applied -- the
same task id means a
+ * different run of it in every pass -- so it switches where the current one
is named. Picking one
+ * rewrites the query rather than the path, so the level links to itself.
+ */
+export const IterationSwitcherButton = ({ children, includeWholeLoop = false,
options, shape }: Props) => {
+ const { t: translate } = useTranslation("common");
+ const [open, setOpen] = useState(false);
+ const [searchParams, setSearchParams] = useSearchParams();
+ const { pathname } = useLocation();
+
+ // A task group can be looked at whole, so it keeps the loop-wide entry; a
task instance is
+ // one row and must name a pass. Passes arrive newest first, so the newest
leads.
+ const passes = includeWholeLoop ? options : options.filter((option) =>
option.iteration !== undefined);
+ // A table page carries the pass as a filter, a task instance page as a
region coordinate.
+ // Either names the same pass, so whichever is present decides what the
level is showing.
+ // A pass belongs to the loop that counted it, so one carried in from a
different loop is
+ // dropped rather than read as a position in this one.
+ const carriedLoop = searchParams.get(SearchParamsKeys.LOOP_ID);
+ const sameLoop = carriedLoop === null || carriedLoop === "" || carriedLoop
=== passes[0]?.loopId;
+ const active = sameLoop
+ ? (searchParams.get(SearchParamsKeys.ITERATION) ??
searchParams.get(SearchParamsKeys.REGION_INDEX))
+ : null;
+ const isActive = (option: LoopOption) =>
+ active === null || active === "" ? option === passes[0] :
String(option.iteration) === active;
+
+ const choose = (option: LoopOption) => {
+ const next = new URLSearchParams(searchParams);
+
+ next.set(SearchParamsKeys.LOOP_ID, option.loopId);
+ if (option.iteration === undefined) {
+ next.delete(SearchParamsKeys.ITERATION);
+ } else {
+ next.set(SearchParamsKeys.ITERATION, String(option.iteration));
+ }
+ // A looped task has one row per pass, so the pass has to move the
coordinate as well as the
+ // filter: without it a task instance page would keep showing the row it
was already on.
+ if (option.loopRegionId === undefined || option.iteration === undefined) {
+ next.delete(SearchParamsKeys.REGION_ID);
+ next.delete(SearchParamsKeys.REGION_INDEX);
+ } else {
+ next.set(SearchParamsKeys.REGION_ID, option.loopRegionId);
Review Comment:
`option.loopRegionId` is the loop summary's family root (`loop_position`
walks `forked_from_region_id` back to the root), but the single-TI endpoints
match `TI.region_id` exactly. After a clear that forks the later iterations,
passes past the cut live in the successor region, so picking one of them here
gives a 404. For a mapped task in a loop the row's own region is the expansion
rather than the loop, so this pair doesn't name a row on the mapped page
either. Could the loop summary expose each pass's live region, or the TI page
resolve (loop, pass) on the server?
This also keeps `cursor` and `offset`, which the old `IterationSelect`
reset, so the table pages from a stale cursor after switching passes.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##########
@@ -442,21 +442,43 @@ def _build_ti_summaries(
if isinstance(group, SerializedLoopTaskGroup)
}
regions = DynamicRegion.load_for_run(dag_id, run_id, session=session) if
loop_ids else {}
- for ti in chain([first], rows):
+ positions_by_ti = [
+ (
+ ti,
+ {
+ loop_id: position
+ for loop_id in loop_ids
+ if (position := loop_position(regions, ti.region_id,
ti.region_index, loop_id)) is not None
+ },
+ )
+ for ti in chain([first], rows)
+ ]
+ # A looped task has one row per iteration. Counting them all would report
the work done
+ # across the whole loop rather than the state the task is in, so the state
counts come from
+ # the newest iteration of each invocation. Every iteration still feeds
loop_iterations,
+ # which is what reports how many ran.
+ latest_iteration: dict[tuple[str, str, UUID], int] = {}
+ for ti, positions in positions_by_ti:
+ for loop_id, (family_id, iteration) in positions.items():
+ key = (ti.task_id, loop_id, family_id)
+ if iteration > latest_iteration.get(key, -1):
+ latest_iteration[key] = iteration
+ for ti, positions in positions_by_ti:
summary = ti_details.get(ti.task_id)
if summary is None:
summary = ti_details[ti.task_id] = GridNodeAgg()
- loop_positions = {}
- for loop_id in loop_ids:
- if position := loop_position(regions, ti.region_id,
ti.region_index, loop_id):
- loop_positions[loop_id] = position
summary.add_ti(
state=ti.state,
start_date=ti.start_date,
end_date=ti.end_date,
dag_version_number=getattr(ti, "version_number", None),
has_note=bool(getattr(ti, "has_note", False)),
- loop_positions=loop_positions,
+ loop_positions=positions,
+ region=(ti.region_id, ti.region_index) if positions else None,
+ count_state=all(
Review Comment:
Counting only the newest pass hides an earlier pass that's re-running. The
Clear dialog lets you clear pass 1 without later iterations, and then its new
attempt is queued or running while pass 3 is still success, so the cell and its
group stay green and the clear looks like it did nothing. Maybe let
non-terminal states from any pass count? Nothing tests `count_state`,
`latest_region_id`/`latest_region_index` or `task_count` yet.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_instances.py:
##########
@@ -614,10 +615,15 @@ def get_task_instances(
"data_interval_end",
"rendered_map_index",
"operator",
+ "iteration",
],
TI,
to_replace={
"map_index": public_map_index_expression(TI),
+ # A loop member's pass number is its own region index.
Rows a loop did not
+ # produce have no pass, and a mapped expansion nested in
one is indexed by
+ # its slot, so this orders meaningfully alongside a
``loop_id`` filter.
+ "iteration": TI.region_index,
Review Comment:
Sorting by `iteration` with a cursor raises. The next cursor is built from a
`TaskCoordinateView`, and for a column-form replacement `SortParam.row_value`
does `getattr(row, "iteration")`. `TaskCoordinateView.__getattr__` has no
`iteration` branch and `TaskInstance` has no such attribute, so it ends in the
`NotImplementedError` in `sort.py`. The UI always sends `cursor` and makes the
Iteration column sortable, so sorting any run with more than one page fails.
It also doesn't sort by what the column shows: for a mapped task inside a
loop `region_index` is the map slot, while the cell renders
`loop_iteration.iteration`. Could this sort on the enclosing pass, with a
matching `iteration` branch on `TaskCoordinateView`? The new test pages by
offset, so a cursor case with a mapped-in-loop row would pin it.
`v2-rest-api-generated.yaml` also still lists the old sort keys without
`iteration`.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/execution.py:
##########
@@ -51,8 +53,19 @@ def get_execution(
dag_id: str,
dag_run_id: str,
session: SessionDep,
+ resolver: CoordinateResolverDep,
limit: QueryLimit,
offset: QueryOffset,
+ order_by: Annotated[
+ SortParam,
+ Depends(
+ SortParam(
+ ["id", "task_id", "map_index", "state", "try_number",
"start_date", "end_date", "duration"],
+ TI,
+ to_replace={"map_index": "region_index"},
Review Comment:
Nothing in the UI calls `getExecution` after this PR (only cache
invalidations reference its key), yet the route gains `order_by`, `in_loop` and
region nesting. `order_by=map_index` always 400s, since the `region_index`
alias isn't in the allowed attrs, and the stable default ordering in
`get_execution_members` can't be reached from here any more. Should the route
changes (or the route) go, along with the now unused `execution` block in
`en/dag.json`?
##########
airflow-core/src/airflow/ui/src/queries/useLoopFilterOptions.ts:
##########
@@ -0,0 +1,170 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueries } from "@tanstack/react-query";
+
+import { UseGridServiceGetLoopSummaryKeyFn } from "openapi/queries/common";
+import { GridService } from "openapi/requests/services.gen";
+import type { TaskInstanceState } from "openapi/requests/types.gen";
+
+import { SearchParamsKeys } from "src/constants/searchParams";
+import { useLoopGroupIds } from "src/queries/useIsLoopGroup";
+
+// Loop ids are dotted task group paths, so "::" cannot occur inside one and
can join the two
+// halves of an option value.
+const SEPARATOR = "::";
+
+export const encodeLoopOption = (loopId: string, iteration?: number): string =>
+ iteration === undefined ? loopId : `${loopId}${SEPARATOR}${iteration}`;
+
+export const decodeLoopOption = (value: string): { iteration?: string; loopId:
string } => {
+ const index = value.lastIndexOf(SEPARATOR);
+
+ return index === -1
+ ? { loopId: value }
+ : { iteration: value.slice(index + SEPARATOR.length), loopId:
value.slice(0, index) };
+};
+
+/**
+ * The loops a page should offer: all of them, or just the one a task group is
part of.
+ *
+ * On a group's own page only that group's loop is in view. A group id is a
dotted path, so a
+ * group nested inside a loop is prefixed by it and reports the loop that
encloses it.
+ */
+export const loopsInScope = (declaredLoops: Array<string>, groupId?: string):
Array<string> =>
+ groupId === undefined
+ ? declaredLoops
+ : declaredLoops.filter((id) => id === groupId ||
groupId.startsWith(`${id}.`));
+
+/**
+ * The pass a page is showing, given the loop's options and the url that
addressed it.
+ *
+ * A table carries the pass as a filter and a task instance as a region
coordinate; either names
+ * the same pass. A pass belongs to the loop that counted it, so one carried
over from a different
+ * loop is ignored rather than read as a position in this one. With nothing
pinned the newest
+ * pass is what the page means, since that is the one being read.
+ */
+export const selectActivePass = (
+ options: Array<LoopOption>,
+ searchParams: URLSearchParams,
+): LoopOption | undefined => {
+ const [, ...passes] = options;
+ const carriedLoop = searchParams.get(SearchParamsKeys.LOOP_ID);
+
+ if (carriedLoop !== null && carriedLoop !== "" && carriedLoop !==
passes[0]?.loopId) {
+ return passes[0];
+ }
+
+ const pinned =
+ searchParams.get(SearchParamsKeys.ITERATION) ??
searchParams.get(SearchParamsKeys.REGION_INDEX);
+
+ return pinned === null || pinned === ""
+ ? passes[0]
+ : passes.find((pass) => String(pass.iteration) === pinned);
+};
+
+export type LoopOption = {
+ // Set only on the whole-loop entry, which reports the run rather than one
pass.
+ iteration: number | undefined;
+ iterationsRan: number | undefined;
+ loopId: string;
+ // The loop's own region. With a pass index it addresses that pass's row
exactly, which is
+ // what a task instance page needs to show the pass rather than merely
filter by it.
+ loopRegionId: string | undefined;
+ maxIterations: number | undefined;
+ state: TaskInstanceState | undefined;
+ value: string;
+};
+
+// A loop's outcome is its own vocabulary, but it lands on the same badge as a
task instance so
+// the whole-loop entry reads like the passes beneath it. Both
terminal-but-unremarkable endings
+// -- it converged, or it used up its allowance -- are a loop that did its job.
+const LOOP_STATE: Record<string, TaskInstanceState> = {
+ failed: "failed",
+ ran_to_cap: "success",
+ removed: "removed",
+ running: "running",
+ skipped: "skipped",
+ stopped_early: "success",
+};
+
+/**
+ * Every loop in the run paired with the passes it actually ran, as one flat
list.
+ *
+ * Each loop reports its own iterations, so this fans out over them; a Dag
normally declares one.
+ */
+export const useLoopFilterOptions = ({
+ dagId,
+ groupId,
+ runId,
+}: {
+ dagId?: string;
+ groupId?: string;
+ runId?: string;
+}) => {
+ const loopGroupIds = loopsInScope(useLoopGroupIds(), groupId);
+ const enabled = dagId !== undefined && runId !== undefined;
+
+ return useQueries({
+ combine: (results) => ({
+ isFetching: results.some((result) => result.isFetching),
+ options: results.flatMap((result, position): Array<LoopOption> => {
+ const loopId = loopGroupIds[position] ?? "";
+ // The whole-loop entry stands on its own, so a loop still filters
before any
+ // iteration has been reported.
+ const all: LoopOption = {
+ iteration: undefined,
+ iterationsRan: result.data?.iterations_ran,
+ loopId,
+ loopRegionId: result.data?.loop_region_id ?? undefined,
+ maxIterations: result.data?.max_iterations,
+ state: result.data === undefined ? undefined :
LOOP_STATE[result.data.status],
+ value: encodeLoopOption(loopId),
+ };
+
+ return [
+ all,
+ // The service reports passes oldest-first, but the newest is the
one being read, so
+ // it leads the list and sits first after the whole-loop entry.
+ ...[...(result.data?.iterations ?? [])].reverse().map((iteration) =>
({
+ iteration: iteration.index,
+ iterationsRan: undefined,
+ loopId,
+ loopRegionId: result.data?.loop_region_id ?? undefined,
+ maxIterations: undefined,
+ state: iteration.state ?? undefined,
+ value: encodeLoopOption(loopId, iteration.index),
+ })),
+ ];
+ }),
+ }),
+ // A pass belongs to one run, so with no run in scope there is nothing to
offer. Without this
+ // the whole-loop entry is still built from an unresolved query, and
callers that key off
+ // ``options.length`` -- the breadcrumb's iteration level among them --
would show it anyway.
+ queries: (enabled ? loopGroupIds : []).map((loopId) => ({
Review Comment:
These queries have no `refetchInterval`, and the global defaults are a 5
minute `staleTime` with window-focus refetch off, so on TI pages the breadcrumb
passes, the switcher and the filter options don't update while a loop is
running. The old group selector used the polling `useLoopSummary`; could this
reuse that?
##########
airflow-core/src/airflow/ui/src/pages/TaskInstances/TaskInstances.tsx:
##########
@@ -401,12 +410,14 @@ export const TaskInstances = ({ extraFilter, iteration,
loopGroupId }: TaskInsta
durationGte: durationGte !== null && durationGte !== "" ?
Number(durationGte) : undefined,
durationLte: durationLte !== null && durationLte !== "" ?
Number(durationLte) : undefined,
endDateLte: endDate ?? undefined,
- iteration:
- inLoopRun && iteration !== undefined && iteration !== ITERATION_ALL ?
Number(iteration) : undefined,
+ // The loop is named by the author; the server turns that into the
regions behind it.
+ // A pass number is meaningless without the loop that counts it, and the
endpoint rejects
+ // the pair outright, so a stale iteration left in the URL is dropped
rather than sent.
+ iteration: loopInScope && Number.isInteger(parsedIteration) ?
parsedIteration : undefined,
limit: pagination.pageSize,
logicalDateGte: logicalDateGte ?? undefined,
logicalDateLte: logicalDateLte ?? undefined,
- loopId: runId === undefined ? undefined : loopGroupId,
+ loopId: loopInScope ? loopFilter : undefined,
Review Comment:
This used to send `loopId` only when `runId` was set. Without that guard,
the cross-run Task page sends `dag_run_id="~"` with a `loop_id`, which the
endpoint rejects with a 400. `TaskNames` links to `/dags/{d}/tasks/{t}` and now
keeps `loop_id`/`iteration`, so opening that page's Task Instances tab from
inside a loop fails, and the loop pill that would clear the filter is hidden
there. Could this go back to sending the loop filter only with a `runId`?
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_instances.py:
##########
@@ -1503,22 +1503,81 @@ def body():
)
session.commit()
url = f"/dags/{dr.dag_id}/dagRuns/{dr.run_id}/taskInstances"
- params = {"loop_id": "body", "limit": 1}
+ params = {"region_id": str(root_id), "limit": 1}
Review Comment:
This test used to cover the `loop_id` + `iteration` filter and now uses
`region_id`, and `test_iteration_filter_requires_loop` was deleted. The new UI
iteration filter sends exactly the `loop_id` + `iteration` pair, so could that
coverage come back alongside this one?
##########
airflow-core/src/airflow/ui/src/queries/useLoopSummary.ts:
##########
@@ -51,7 +51,7 @@ export const useLoopSummary = ({
{
dagId,
groupId,
- loopRegionId: searchParams.get(SearchParamsKeys.LOOP_REGION_ID) ??
undefined,
+ loopRegionId: searchParams.get(SearchParamsKeys.REGION_ID) ?? undefined,
Review Comment:
On a TI page this sends the TI's own `region_id` as `loop_region_id`, and
`summarize_loop_run` 404s for anything that isn't a family root. So on a forked
pass, or a mapped task inside a loop, the summary 404s and the header falls
back to `k/k`. Separately, the header numerator is the 0-based pass index over
a count, so the last of 5 passes reads "4/5". Sending the family id (or no
region) and showing `iteration + 1` would fix both.
##########
airflow-core/src/airflow/ui/src/pages/MappedTaskInstance/Header.tsx:
##########
@@ -60,6 +66,14 @@ export const Header = ({ taskInstance }: { readonly
taskInstance: LightGridTaskI
});
const stats = [
...entries,
+ ...(pass === undefined
+ ? []
+ : [
+ {
+ label: translate("common:filters.iteration"),
+ value: `${pass.iteration}/${loopOptions[0]?.iterationsRan ??
pass.iteration}`,
Review Comment:
The counts and state in this header come from the run-level grid summary,
which only counts the newest pass, but the Iteration label comes from whichever
pass the URL pins. Pin pass 0 with 100 slots while pass 1 has 2 slots, and this
shows 2 under iteration 0. Scope the aggregate to the selected pass, or drop
the label here?
##########
airflow-core/src/airflow/ui/src/system-components/Modal/Modal.test.tsx:
##########
@@ -102,9 +102,31 @@ describe("Modal", () => {
expect([...(footer?.querySelectorAll("button") ?? [])].map((button) =>
button.textContent)).toStrictEqual(
["Save", "modal.cancel"],
);
- expect(globalThis.getComputedStyle(footer?.firstElementChild as
HTMLElement).flexDirection).toBe(
- "row-reverse",
+ // ``row-reverse`` sits on the group holding the actions, not the footer's
own first child:
+ // the footer leads with the options group, which is left-aligned and
empty here.
+ const actionsGroup = screen.getByRole("button", { name: "Save"
}).parentElement;
+
+ expect(globalThis.getComputedStyle(actionsGroup as
HTMLElement).flexDirection).toBe("row-reverse");
+ });
+
+ it("puts footerOptions at the left of the footer, ahead of the actions", ()
=> {
+ render(
+ <Modal
+ footerActions={<Button>Save</Button>}
+ footerOptions={<span>Keep state</span>}
+ open
+ title="Modal Heading"
+ >
+ Lorem Ipsum Content
+ </Modal>,
+ { wrapper: BaseWrapper },
);
+
+ const footer = screen.getByRole("button", { name: "Save"
}).closest(".chakra-dialog__footer");
+ const option = screen.getByText("Keep state");
+
+ // The options group precedes the actions group in the document, so it
renders left of it.
+ expect(footer?.firstElementChild?.contains(option)).toBe(true);
Review Comment:
`footer.firstElementChild` is the outer `Flex` that wraps both groups, so
`contains(option)` is true whatever the order. Checking against the options
group itself, or comparing document position with the Save button, would make
this fail if the order flipped.
##########
airflow-core/src/airflow/ui/src/queries/useLoopFilterOptions.ts:
##########
@@ -0,0 +1,170 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueries } from "@tanstack/react-query";
+
+import { UseGridServiceGetLoopSummaryKeyFn } from "openapi/queries/common";
+import { GridService } from "openapi/requests/services.gen";
+import type { TaskInstanceState } from "openapi/requests/types.gen";
+
+import { SearchParamsKeys } from "src/constants/searchParams";
+import { useLoopGroupIds } from "src/queries/useIsLoopGroup";
+
+// Loop ids are dotted task group paths, so "::" cannot occur inside one and
can join the two
+// halves of an option value.
+const SEPARATOR = "::";
+
+export const encodeLoopOption = (loopId: string, iteration?: number): string =>
+ iteration === undefined ? loopId : `${loopId}${SEPARATOR}${iteration}`;
+
+export const decodeLoopOption = (value: string): { iteration?: string; loopId:
string } => {
+ const index = value.lastIndexOf(SEPARATOR);
+
+ return index === -1
+ ? { loopId: value }
+ : { iteration: value.slice(index + SEPARATOR.length), loopId:
value.slice(0, index) };
+};
+
+/**
+ * The loops a page should offer: all of them, or just the one a task group is
part of.
+ *
+ * On a group's own page only that group's loop is in view. A group id is a
dotted path, so a
+ * group nested inside a loop is prefixed by it and reports the loop that
encloses it.
+ */
+export const loopsInScope = (declaredLoops: Array<string>, groupId?: string):
Array<string> =>
+ groupId === undefined
+ ? declaredLoops
+ : declaredLoops.filter((id) => id === groupId ||
groupId.startsWith(`${id}.`));
Review Comment:
Loop membership is decided by the id prefix here (and the same way for the
Gantt group highlight). `prefix_group_id=False` isn't rejected for loops, since
`tg_kwargs` pass straight through in `loop.py`, so members of such a loop lose
the breadcrumb, filter and highlight. Is that combination meant to be supported?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
