choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2384919971
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py:
##########
@@ -79,6 +79,7 @@ class GridRunsResponse(BaseModel):
run_after: datetime
state: TaskInstanceState | None
run_type: DagRunType
+ dag_version_number: int | None = None
Review Comment:
Although `dag_versions` is defined as a list in the DagRun model, we don’t
need to pass the entire list. For the indicator, we only need the latest
version of the DagRun, so passing just the latest version number is sufficient.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx:
##########
@@ -36,26 +36,56 @@ export const TaskInstancesColumn = ({ nodes, runId,
taskInstances }: Props) => {
const [searchParams] = useSearchParams();
const search = searchParams.toString();
- return nodes.map((node) => {
+ return nodes.map((node, idx) => {
// todo: how does this work with mapped? same task id for multiple tis
const taskInstance = taskInstances.find((ti) => ti.task_id === node.id);
if (!taskInstance) {
return <Box height="20px" key={`${node.id}-${runId}`} width="18px" />;
}
+ // Check if dag_version changed compared to previous task
+ const prevNode = idx > 0 ? nodes[idx - 1] : undefined;
+ const prevTaskInstance = prevNode ? taskInstances.find((ti) => ti.task_id
=== prevNode.id) : undefined;
+
+ const showVersionDivider = Boolean(
+ prevTaskInstance &&
+ taskInstance.dag_version_id !== undefined &&
+ prevTaskInstance.dag_version_id !== undefined &&
+ taskInstance.dag_version_id !== prevTaskInstance.dag_version_id,
+ );
+
return (
- <GridTI
- dagId={dagId}
- isGroup={node.isGroup}
- isMapped={node.is_mapped}
- key={node.id}
- label={node.label}
- runId={runId}
- search={search}
- state={taskInstance.state}
- taskId={node.id}
- />
+ <Box key={`${node.id}-${runId}`} position="relative">
+ {showVersionDivider ? (
+ <Box bg="orange.400" height="2px" left="0" position="absolute"
top="-1px" width="18px" zIndex={3}>
+ <Text
+ bg="white"
+ borderRadius="2px"
+ color="orange.700"
+ fontSize="8px"
+ position="absolute"
+ px="1px"
+ right="-8px"
+ top="-4px"
+ whiteSpace="nowrap"
+ >
+ {`v${taskInstance.dag_version_number ?? ""}`}
Review Comment:
I created VersionIndicator.tsx to unify the font, background, and colors so
that the horizontal and vertical version dividers look exactly the same.
##########
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##########
@@ -0,0 +1,154 @@
+/*!
+ * 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 { Box, Text } from "@chakra-ui/react";
+import { FiGitCommit } from "react-icons/fi";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => (
+ <Box
+ color="orange.fg"
+ left="-8px"
+ position="absolute"
+ title={`Bundle Version: ${bundleVersion}`}
+ top="93px"
+ >
+ <FiGitCommit size="15px" />
+ </Box>
+);
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+ <Box
+ aria-label={`Version ${dagVersionNumber} indicator`}
+ as="output"
+ height={isVertical ? "98px" : "2px"}
+ left={isVertical ? "-1px" : "0"}
+ position="absolute"
+ top={isVertical ? "0" : "-1px"}
+ width={isVertical ? "1.75px" : "18px"}
+ zIndex={3}
+ >
+ {isVertical ? (
+ <>
+ <Box bg="orange.focusRing" height="100%" left="0"
position="absolute" top="0" width="1.75px" />
+
+ <Box
+ _hover={{ "& > .version-tooltip": { opacity: 1, visibility:
"visible" } }}
+ left="50%"
+ position="absolute"
+ top="-7px"
Review Comment:
When trying to display an indicator between the bars of a bar chart, there
are limitations when using Chakra UI tokens. What would be a good approach in
this case?
##########
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##########
@@ -0,0 +1,154 @@
+/*!
+ * 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 { Box, Text } from "@chakra-ui/react";
+import { FiGitCommit } from "react-icons/fi";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => (
+ <Box
+ color="orange.fg"
+ left="-8px"
+ position="absolute"
+ title={`Bundle Version: ${bundleVersion}`}
+ top="93px"
+ >
+ <FiGitCommit size="15px" />
+ </Box>
+);
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+ <Box
+ aria-label={`Version ${dagVersionNumber} indicator`}
+ as="output"
+ height={isVertical ? "98px" : "2px"}
+ left={isVertical ? "-1px" : "0"}
+ position="absolute"
+ top={isVertical ? "0" : "-1px"}
+ width={isVertical ? "1.75px" : "18px"}
+ zIndex={3}
+ >
+ {isVertical ? (
+ <>
+ <Box bg="orange.focusRing" height="100%" left="0"
position="absolute" top="0" width="1.75px" />
+
+ <Box
+ _hover={{ "& > .version-tooltip": { opacity: 1, visibility:
"visible" } }}
+ left="50%"
+ position="absolute"
+ top="-7px"
+ transform="translateX(-50%)"
+ zIndex={5}
Review Comment:
I changed it to use the tooltip token.
##########
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##########
@@ -0,0 +1,155 @@
+/*!
+ * 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 { Box, Text } from "@chakra-ui/react";
+import { FiGitCommit } from "react-icons/fi";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => (
+ <Box
+ color="orange.fg"
+ left={-2}
+ position="absolute"
+ title={`Bundle Version: ${bundleVersion}`}
Review Comment:
I’ve updated it. Thanks!
##########
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##########
@@ -0,0 +1,155 @@
+/*!
+ * 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 { Box, Text } from "@chakra-ui/react";
+import { FiGitCommit } from "react-icons/fi";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => (
+ <Box
+ color="orange.fg"
+ left={-2}
+ position="absolute"
+ title={`Bundle Version: ${bundleVersion}`}
+ top={93}
+ zIndex={1}
+ >
+ <FiGitCommit size="15px" />
+ </Box>
+);
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+ <Box
+ aria-label={`Version ${dagVersionNumber} indicator`}
+ as="output"
+ height={isVertical ? 104 : 0.5}
+ left={isVertical ? -1.25 : 0}
+ position="absolute"
+ top={isVertical ? -1.5 : -0.5}
+ width={isVertical ? 0.5 : 4.5}
+ zIndex={1}
+ >
+ {isVertical ? (
+ <>
+ <Box bg="orange.focusRing" height="full" position="absolute"
width={0.5} />
+
+ <Box
+ _hover={{ "& > .version-tooltip": { opacity: 1, visibility:
"visible" } }}
+ left="50%"
+ position="absolute"
+ top={-2}
+ transform="translateX(-50%)"
+ zIndex="tooltip"
+ >
+ <Box
+ _hover={{
+ bg: "orange.emphasis",
+ cursor: "pointer",
+ }}
+ bg="orange.focusRing"
+ borderRadius="full"
+ height={2}
+ transition="all 0.2s"
+ width={2}
+ />
+ <Box
+ bg="orange.focusRing"
+ borderRadius="full"
+ className="version-tooltip"
+ left="50%"
+ opacity={0}
+ pointerEvents="none"
+ position="absolute"
+ px={0.5}
+ py={0.5}
+ top={-0.5}
+ transform="translateX(-50%)"
+ transition="all 0.2s"
+ visibility="hidden"
+ >
+ <Text
+ color={{ _dark: "black", _light: "white" }}
Review Comment:
I’ve updated it. Thanks!
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/Bar.tsx:
##########
@@ -34,17 +35,25 @@ type Props = {
readonly nodes: Array<GridTask>;
readonly onCellClick?: () => void;
readonly onColumnClick?: () => void;
- readonly run: GridRunsResponse;
+ readonly run: {
+ has_mixed_versions?: boolean;
Review Comment:
yes that's right I fix it thanks ;)
##########
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx:
##########
@@ -100,6 +102,16 @@ const getWidthBasedConfig = (width: number,
enableResponsiveOptions: boolean) =>
};
};
+const getVersionDisplayOptions = (translate: (key: string) => string) =>
+ createListCollection({
+ items: [
+ { label: translate("dag:panel.versionDisplay.options.showAll"), value:
"all" },
+ { label: translate("dag:panel.versionDisplay.options.showDagVersion"),
value: "dag" },
+ { label:
translate("dag:panel.versionDisplay.options.showBundleVersion"), value:
"bundle" },
+ { label: translate("dag:panel.versionDisplay.options.hideAll"), value:
"none" },
Review Comment:
Goood! I’ve applied that in
`airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts`!
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx:
##########
@@ -20,41 +20,75 @@ import { Box } from "@chakra-ui/react";
import { useParams } from "react-router-dom";
import type { LightGridTaskInstanceSummary } from "openapi/requests/types.gen";
+import { DagVersionIndicator } from "src/components/ui/VersionIndicator";
import { GridTI } from "./GridTI";
import type { GridTask } from "./utils";
type Props = {
readonly depth?: number;
+ readonly hasMixedVersions?: boolean;
readonly nodes: Array<GridTask>;
readonly onCellClick?: () => void;
readonly runId: string;
readonly taskInstances: Array<LightGridTaskInstanceSummary>;
+ readonly versionDisplayMode?: string;
};
-export const TaskInstancesColumn = ({ nodes, onCellClick, runId, taskInstances
}: Props) => {
+export const TaskInstancesColumn = ({
+ hasMixedVersions,
+ nodes,
+ onCellClick,
+ runId,
+ taskInstances,
+ versionDisplayMode,
+}: Props) => {
const { dagId = "" } = useParams();
- return nodes.map((node) => {
+ const taskInstanceMap = new Map(taskInstances.map((ti) => [ti.task_id, ti]));
Review Comment:
I remember switching to a Map because there was a noticeable performance
difference at the time.
It looks like the internal logic (taskInstance) has improved since then, and
now the difference is almost negligible (under 0.1ms in 50 tasks).
Since this only increases code complexity, I think it's better to revert
back to the original Array.find() approach and add the needed functionality on
top of that.
##########
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##########
@@ -0,0 +1,155 @@
+/*!
+ * 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 { Box, Text } from "@chakra-ui/react";
+import { FiGitCommit } from "react-icons/fi";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => (
+ <Box
+ color="orange.fg"
+ left={-2}
+ position="absolute"
+ title={`Bundle Version: ${bundleVersion}`}
+ top={93}
+ zIndex={1}
+ >
+ <FiGitCommit size="15px" />
+ </Box>
+);
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+ <Box
+ aria-label={`Version ${dagVersionNumber} indicator`}
+ as="output"
+ height={isVertical ? 104 : 0.5}
+ left={isVertical ? -1.25 : 0}
+ position="absolute"
+ top={isVertical ? -1.5 : -0.5}
+ width={isVertical ? 0.5 : 4.5}
+ zIndex={1}
+ >
+ {isVertical ? (
+ <>
+ <Box bg="orange.focusRing" height="full" position="absolute"
width={0.5} />
+
+ <Box
+ _hover={{ "& > .version-tooltip": { opacity: 1, visibility:
"visible" } }}
+ left="50%"
+ position="absolute"
+ top={-2}
+ transform="translateX(-50%)"
+ zIndex="tooltip"
+ >
+ <Box
+ _hover={{
+ bg: "orange.emphasis",
+ cursor: "pointer",
+ }}
+ bg="orange.focusRing"
+ borderRadius="full"
+ height={2}
+ transition="all 0.2s"
+ width={2}
+ />
+ <Box
Review Comment:
I used a custom tooltip to prevent it from covering other task status boxes
in the Task Instance grid view, allowing it to naturally overlap with the
indicator.
However, this approach made the CSS more complex and harder to maintain, so
I’ve been reconsidering it.
As you suggested, I think it would be better to use the Chakra tooltip
provided in the component for better consistency and simplicity, and I’ll
update it accordingly.
<img width="363" height="278" alt="image"
src="https://github.com/user-attachments/assets/3672d32f-b834-42b4-acd8-f3555c8ebb6f";
/>
<img width="257" height="225" alt="image"
src="https://github.com/user-attachments/assets/d689474f-d3d5-4ba0-af0b-5f475f622b7d";
/>
--
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]
