bbovenzi commented on code in PR #55927:
URL: https://github.com/apache/airflow/pull/55927#discussion_r2414415887
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##########
@@ -348,41 +337,49 @@ def get_grid_ti_summaries(
TaskInstance.end_date,
)
.where(TaskInstance.dag_id == dag_id)
- .where(
- TaskInstance.run_id == run_id,
- )
+ .where(TaskInstance.run_id == run_id)
),
filters=[],
order_by=SortParam(allowed_attrs=["task_id", "run_id"],
model=TaskInstance).set_value(["task_id"]),
limit=None,
return_total_entries=False,
)
- task_instances = list(session.execute(tis_of_dag_runs))
- if not task_instances:
+ rows = list(session.execute(tis_of_dag_runs))
+ if not rows:
raise HTTPException(
status.HTTP_404_NOT_FOUND, f"No task instances for dag_id={dag_id}
run_id={run_id}"
)
- ti_details = collections.defaultdict(list)
- for ti in task_instances:
- ti_details[ti.task_id].append(
+
+ # Build TI details per task_id
+ ti_details: dict[str, list[dict[str, object | None]]] =
collections.defaultdict(list)
+ for r in rows:
+ ti_details[r.task_id].append(
{
- "state": ti.state,
- "start_date": ti.start_date,
- "end_date": ti.end_date,
+ "state": r.state,
+ "start_date": r.start_date,
+ "end_date": r.end_date,
Review Comment:
We should probably just add `"duration": ti.duration` here since we ran into
another issue where the UI-calculated duration was out of sync with the actual
duration: https://github.com/apache/airflow/pull/56310
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/DurationTick.tsx:
##########
@@ -17,15 +17,36 @@
* under the License.
*/
import { Text, type TextProps } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
-import { renderDuration } from "src/utils";
+import type { LightGridTaskInstanceSummary, TaskInstanceResponse } from
"openapi/requests/types.gen";
+import TaskInstanceTooltip from "src/components/TaskInstanceTooltip";
type Props = {
readonly duration: number;
+ readonly fullInstance?: TaskInstanceResponse;
+ /**
+ * The lightweight or full task instance for this tick.
+ * If both are available, pass fullInstance; otherwise pass instance.
+ */
+ readonly instance?: LightGridTaskInstanceSummary;
} & TextProps;
-export const DurationTick = ({ duration, ...rest }: Props) => (
- <Text color="border.emphasized" fontSize="xs" position="absolute" right={1}
whiteSpace="nowrap" {...rest}>
- {renderDuration(duration)}
- </Text>
-);
+export const DurationTick = ({ duration, fullInstance, instance, ...rest }:
Props) => {
+ const { t: translate } = useTranslation();
+
+ // Prefer the full instance when available
+ const taskInstance = fullInstance ?? instance;
+
+ const tickLabel = (
+ <Text color="border.emphasized" fontSize="xs" position="absolute"
right={1} whiteSpace="nowrap" {...rest}>
+ {translate("seconds", { count: Math.floor(duration) })}
Review Comment:
This is also a translation that doesn't exist. If you're using an LLM,
please check that its not inventing values that don't exist.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/GridTI.tsx:
##########
@@ -84,6 +84,18 @@ const Instance = ({ dagId, instance, isGroup, isMapped,
onClick, runId, search,
[dagId, isGroup, isMapped, location.pathname, runId, taskId],
);
+ const start = instance.min_start_date ?? undefined;
+ const end = instance.max_end_date ?? undefined;
+
+ const hasStart = start !== undefined;
+ const hasEnd = end !== undefined;
+
+ const seconds =
+ hasStart && hasEnd ? dayjs(end).diff(dayjs(start), "second", true) :
undefined;
+
+ const durationText =
+ seconds === undefined ? translate("notAvailable", { defaultValue: "--" })
: renderDuration(seconds) ?? translate("notAvailable", { defaultValue: "--" });
Review Comment:
I don't think `"notAvailable"` is a real key, nor should we even use it. I
think showing nothing is fine.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##########
@@ -348,41 +337,49 @@ def get_grid_ti_summaries(
TaskInstance.end_date,
)
.where(TaskInstance.dag_id == dag_id)
- .where(
- TaskInstance.run_id == run_id,
- )
+ .where(TaskInstance.run_id == run_id)
),
filters=[],
order_by=SortParam(allowed_attrs=["task_id", "run_id"],
model=TaskInstance).set_value(["task_id"]),
limit=None,
return_total_entries=False,
)
- task_instances = list(session.execute(tis_of_dag_runs))
- if not task_instances:
+ rows = list(session.execute(tis_of_dag_runs))
+ if not rows:
raise HTTPException(
status.HTTP_404_NOT_FOUND, f"No task instances for dag_id={dag_id}
run_id={run_id}"
)
- ti_details = collections.defaultdict(list)
- for ti in task_instances:
- ti_details[ti.task_id].append(
+
+ # Build TI details per task_id
+ ti_details: dict[str, list[dict[str, object | None]]] =
collections.defaultdict(list)
+ for r in rows:
+ ti_details[r.task_id].append(
Review Comment:
There are a lot of unnecessary code changes here. Please remove them so this
PR only has changes relevant to the GridTaskInstanceSummary
--
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]
