Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bbovenzi merged PR #53216: URL: https://github.com/apache/airflow/pull/53216 -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bugraoz93 commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3961969259 > @bugraoz93 Mind reviewing again to remove your Requested Changes block? Of course Brent, reviewed. Thanks for pinging! :) -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bbovenzi commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3961676960 @bugraoz93 Mind reviewing again to remove your Requested Changes block? -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2837071508
##
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##
@@ -274,17 +274,34 @@ def get_grid_runs(
triggering_user: QueryDagRunTriggeringUserSearch,
) -> list[GridRunsResponse]:
"""Get info about a run for the grid."""
-# Retrieve, sort the previous DAG Runs
-base_query = select(
-DagRun.dag_id,
-DagRun.run_id,
-DagRun.queued_at,
-DagRun.start_date,
-DagRun.end_date,
-DagRun.run_after,
-DagRun.state,
-DagRun.run_type,
-).where(DagRun.dag_id == dag_id)
+# get the highest dag_version_number from TIs for each run
+latest_ti_version = (
+select(
+TaskInstance.run_id,
+func.max(DagVersion.version_number).label("version_number"),
+)
+.join(DagVersion, TaskInstance.dag_version_id == DagVersion.id)
+.where(TaskInstance.dag_id == dag_id)
+.group_by(TaskInstance.run_id)
+.subquery()
+)
Review Comment:
Removed the subquery approach and now directly use `DagRun.dag_versions`,
which returns a list via the model property.
The GridRunsResponse model now returns `list[DagVersionResponse]` instead of
a single `dag_version_number`.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2837058751 ## airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py: ## @@ -79,6 +79,8 @@ class GridRunsResponse(BaseModel): run_after: datetime state: DagRunState | None run_type: DagRunType +bundle_version: str | None = None Review Comment: We now retrieve the dag_versions list, which already contains the bundle version. So bundle_version here was redundant and has been removed. -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2831657365
##
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -79,6 +80,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
);
const [showGantt, setShowGantt] =
useLocalStorage(`show_gantt-${dagId}`, false);
+ // Global setting: applies to all Dags (intentionally not scoped to dagId)
+ const [showVersionIndicatorMode, setShowVersionIndicatorMode] =
+useLocalStorage(
+ `version_indicator_display_mode`,
Review Comment:
I agree! It would make the usage more consistent and easier to manage.
I’ll clean this up in a follow-up PR 👍
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2831652841
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,46 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export enum VersionIndicatorDisplayOptions {
Review Comment:
`VersionIndicatorOptions` looks good, Thanks ;)
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2825902766
##
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##
@@ -274,17 +274,34 @@ def get_grid_runs(
triggering_user: QueryDagRunTriggeringUserSearch,
) -> list[GridRunsResponse]:
"""Get info about a run for the grid."""
-# Retrieve, sort the previous DAG Runs
-base_query = select(
-DagRun.dag_id,
-DagRun.run_id,
-DagRun.queued_at,
-DagRun.start_date,
-DagRun.end_date,
-DagRun.run_after,
-DagRun.state,
-DagRun.run_type,
-).where(DagRun.dag_id == dag_id)
+# get the highest dag_version_number from TIs for each run
+latest_ti_version = (
+select(
+TaskInstance.run_id,
+func.max(DagVersion.version_number).label("version_number"),
+)
+.join(DagVersion, TaskInstance.dag_version_id == DagVersion.id)
+.where(TaskInstance.dag_id == dag_id)
+.group_by(TaskInstance.run_id)
+.subquery()
+)
Review Comment:
Thanks for pointing that out.
I initially used the subquery approach to avoid N+1 queries, but I agree
that using `DagRun.dag_versions` is more consistent with the public API and
simplifies the logic. I'll update the implementation accordingly.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2825940327
##
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx:
##
@@ -90,6 +115,23 @@ export const TaskInstancesColumn = ({ nodes, onCellClick,
run, virtualItems }: P
);
}
+let hasVersionChangeFlag = false;
+
+if (
+ hasMixedVersions &&
+ (showVersionIndicatorMode === VersionIndicatorDisplayOptions.DAG ||
+showVersionIndicatorMode === VersionIndicatorDisplayOptions.ALL) &&
+ idx > 0
+) {
+ const prevVirtualItem = itemsToRender[idx - 1];
+ const prevNode = prevVirtualItem ? nodes[prevVirtualItem.index] :
undefined;
+ const prevTaskInstance = prevNode ? taskInstanceMap.get(prevNode.id)
: undefined;
+
+ hasVersionChangeFlag = Boolean(
+prevTaskInstance && prevTaskInstance.dag_version_number !==
taskInstance.dag_version_number,
+ );
+}
+
Review Comment:
bundle_version is a DagRun-level column, so all TaskInstances within a
single run share the same value, as I understand it.
Bundle version changes between runs are already handled in Bar.tsx via
isBundleVersionChange.
Since this comparison operates within a single run, my understanding is that
only `dag_version_number` would differ across TaskInstances.
Let me know if I'm missing something!
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2825902766
##
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##
@@ -274,17 +274,34 @@ def get_grid_runs(
triggering_user: QueryDagRunTriggeringUserSearch,
) -> list[GridRunsResponse]:
"""Get info about a run for the grid."""
-# Retrieve, sort the previous DAG Runs
-base_query = select(
-DagRun.dag_id,
-DagRun.run_id,
-DagRun.queued_at,
-DagRun.start_date,
-DagRun.end_date,
-DagRun.run_after,
-DagRun.state,
-DagRun.run_type,
-).where(DagRun.dag_id == dag_id)
+# get the highest dag_version_number from TIs for each run
+latest_ti_version = (
+select(
+TaskInstance.run_id,
+func.max(DagVersion.version_number).label("version_number"),
+)
+.join(DagVersion, TaskInstance.dag_version_id == DagVersion.id)
+.where(TaskInstance.dag_id == dag_id)
+.group_by(TaskInstance.run_id)
+.subquery()
+)
Review Comment:
Thanks for pointing that out.
I initially used the subquery approach to avoid N+1 queries, but I agree
that using `DagRun.dag_versions` is more consistent with the public API and
simplifies the logic. I'll update the implementation accordingly.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2825886797 ## airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py: ## @@ -79,6 +79,8 @@ class GridRunsResponse(BaseModel): run_after: datetime state: DagRunState | None run_type: DagRunType +bundle_version: str | None = None Review Comment: Yes, it's used in the grid view to detect and display bundle version changes between adjacent Dag runs. `useGridRunsWithVersionFlags.ts` compares `bundle_version` between adjacent runs to compute `isBundleVersionChange` --> `Bar.tsx` conditionally renders `BundleVersionIndicator` when a change is detected -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2825867078
##
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx:
##
@@ -32,25 +35,47 @@ type Props = {
readonly nodes: Array;
readonly onCellClick?: () => void;
readonly run: GridRunsResponse;
+ readonly showVersionIndicatorMode?: VersionIndicatorDisplayOptions;
readonly virtualItems?: Array;
};
const ROW_HEIGHT = 20;
-export const TaskInstancesColumn = ({ nodes, onCellClick, run, virtualItems }:
Props) => {
+export const TaskInstancesColumn = ({
+ nodes,
+ onCellClick,
+ run,
+ showVersionIndicatorMode,
+ virtualItems,
+}: Props) => {
const { dagId = "", runId } = useParams();
const { data: gridTISummaries } = useGridTiSummaries({ dagId, runId:
run.run_id, state: run.state });
const { hoveredRunId, setHoveredRunId } = useHover();
const itemsToRender =
virtualItems ?? nodes.map((_, index) => ({ index, size: ROW_HEIGHT, start:
index * ROW_HEIGHT }));
- const taskInstances = gridTISummaries?.task_instances ?? [];
- const taskInstanceMap = new Map();
+ const taskInstances = useMemo(
+() => gridTISummaries?.task_instances ?? [],
+[gridTISummaries?.task_instances],
+ );
+ const taskInstanceMap = useMemo(() => {
+const map = new Map();
+
+for (const ti of taskInstances) {
+ map.set(ti.task_id, ti);
+}
+
+return map;
+ }, [taskInstances]);
Review Comment:
Cool. I'll remove them :)
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bbovenzi commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2794606530
##
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx:
##
@@ -32,25 +35,47 @@ type Props = {
readonly nodes: Array;
readonly onCellClick?: () => void;
readonly run: GridRunsResponse;
+ readonly showVersionIndicatorMode?: VersionIndicatorDisplayOptions;
readonly virtualItems?: Array;
};
const ROW_HEIGHT = 20;
-export const TaskInstancesColumn = ({ nodes, onCellClick, run, virtualItems }:
Props) => {
+export const TaskInstancesColumn = ({
+ nodes,
+ onCellClick,
+ run,
+ showVersionIndicatorMode,
+ virtualItems,
+}: Props) => {
const { dagId = "", runId } = useParams();
const { data: gridTISummaries } = useGridTiSummaries({ dagId, runId:
run.run_id, state: run.state });
const { hoveredRunId, setHoveredRunId } = useHover();
const itemsToRender =
virtualItems ?? nodes.map((_, index) => ({ index, size: ROW_HEIGHT, start:
index * ROW_HEIGHT }));
- const taskInstances = gridTISummaries?.task_instances ?? [];
- const taskInstanceMap = new Map();
+ const taskInstances = useMemo(
+() => gridTISummaries?.task_instances ?? [],
+[gridTISummaries?.task_instances],
+ );
+ const taskInstanceMap = useMemo(() => {
+const map = new Map();
+
+for (const ti of taskInstances) {
+ map.set(ti.task_id, ti);
+}
+
+return map;
+ }, [taskInstances]);
Review Comment:
Correct. Let's remove them.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bbovenzi commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2794602970
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,46 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export enum VersionIndicatorDisplayOptions {
Review Comment:
`VersionIndicatorOptions` or `VersionDisplayOptions`
We use this a lot. Let's try to have a simpler variable name.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bbovenzi commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2794563055
##
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -79,6 +80,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
);
const [showGantt, setShowGantt] =
useLocalStorage(`show_gantt-${dagId}`, false);
+ // Global setting: applies to all Dags (intentionally not scoped to dagId)
+ const [showVersionIndicatorMode, setShowVersionIndicatorMode] =
+useLocalStorage(
+ `version_indicator_display_mode`,
Review Comment:
I wonder if we should make a `src/constants/localStorage.ts` file to keep
track of how many localStorage values we have.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bbovenzi commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2794542552 ## airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx: ## Review Comment: If we only use this in the grid view then we should move it to `layouts/Details/Grid` instead. `components` are for general reshareable components. (`/ui` was also supposed to be for wrappers that weren't included in chakra-ui but that we hadn't customized, but we haven't been enforcing that too well) -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
pierrejeambrun commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2794142930
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py:
##
@@ -79,6 +79,8 @@ class GridRunsResponse(BaseModel):
run_after: datetime
state: DagRunState | None
run_type: DagRunType
+bundle_version: str | None = None
Review Comment:
is `bundle_version` used?
##
airflow-core/src/airflow/ui/src/layouts/Details/Grid/useGridRunsWithVersionFlags.ts:
##
@@ -0,0 +1,70 @@
+/*!
+ * 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 { useMemo } from "react";
+
+import type { GridRunsResponse } from "openapi/requests";
+import { VersionIndicatorDisplayOptions } from
"src/constants/showVersionIndicatorOptions";
+
+export type GridRunWithVersionFlags = {
+ isBundleVersionChange: boolean;
+ isDagVersionChange: boolean;
+} & GridRunsResponse;
+
+type UseGridRunsWithVersionFlagsParams = {
+ gridRuns: Array | undefined;
+ showVersionIndicatorMode?: VersionIndicatorDisplayOptions;
+};
+
+// Hook to calculate version change flags for grid runs.
+export const useGridRunsWithVersionFlags = ({
+ gridRuns,
+ showVersionIndicatorMode,
+}: UseGridRunsWithVersionFlagsParams): Array |
undefined => {
+ const isVersionIndicatorEnabled = showVersionIndicatorMode !==
VersionIndicatorDisplayOptions.NONE;
+
+ return useMemo(() => {
+if (!gridRuns) {
+ return undefined;
+}
+
+if (!isVersionIndicatorEnabled) {
+ return gridRuns.map((run) => ({ ...run, isBundleVersionChange: false,
isDagVersionChange: false }));
+}
+
+return gridRuns.map((run, index) => {
+ const prevRun = gridRuns[index + 1];
Review Comment:
```suggestion
const nextRun = gridRuns[index + 1];
```
##
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx:
##
@@ -90,6 +115,23 @@ export const TaskInstancesColumn = ({ nodes, onCellClick,
run, virtualItems }: P
);
}
+let hasVersionChangeFlag = false;
+
+if (
+ hasMixedVersions &&
+ (showVersionIndicatorMode === VersionIndicatorDisplayOptions.DAG ||
+showVersionIndicatorMode === VersionIndicatorDisplayOptions.ALL) &&
+ idx > 0
+) {
+ const prevVirtualItem = itemsToRender[idx - 1];
+ const prevNode = prevVirtualItem ? nodes[prevVirtualItem.index] :
undefined;
+ const prevTaskInstance = prevNode ? taskInstanceMap.get(prevNode.id)
: undefined;
+
+ hasVersionChangeFlag = Boolean(
+prevTaskInstance && prevTaskInstance.dag_version_number !==
taskInstance.dag_version_number,
+ );
+}
+
Review Comment:
Why aren't we checking for `BundleVersion` change too ?
DagVersion could be the same (serialized dag is identical) but the bundle
version changed (version of the file in the code)
##
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py:
##
@@ -274,17 +274,34 @@ def get_grid_runs(
triggering_user: QueryDagRunTriggeringUserSearch,
) -> list[GridRunsResponse]:
"""Get info about a run for the grid."""
-# Retrieve, sort the previous DAG Runs
-base_query = select(
-DagRun.dag_id,
-DagRun.run_id,
-DagRun.queued_at,
-DagRun.start_date,
-DagRun.end_date,
-DagRun.run_after,
-DagRun.state,
-DagRun.run_type,
-).where(DagRun.dag_id == dag_id)
+# get the highest dag_version_number from TIs for each run
+latest_ti_version = (
+select(
+TaskInstance.run_id,
+func.max(DagVersion.version_number).label("version_number"),
+)
+.join(DagVersion, TaskInstance.dag_version_id == DagVersion.id)
+.where(TaskInstance.dag_id == dag_id)
+.group_by(TaskInstance.run_id)
+.subquery()
+)
Review Comment:
We don't need to go through all that trouble to only retrieve the 'last'
version. Just return the `DagRun.dag_versions` attribute.
Also this will be consistent for other contributor. People will get confused
as to why a DagRun has `dag_versions`
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2785742054 ## airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx: ## Review Comment: Thanks! I’ve moved this into the grid-specific branch! -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
guan404ming commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2778803444 ## airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx: ## Review Comment: Since version indicators only render in the Grid view, let's move this inside the grid-specific branch (the else block starting at line 342), or conditionally hiding it when dagView === "graph". -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2777668393
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,48 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export enum VersionIndicatorDisplayOptions {
+ ALL = "all",
+ BUNDLE = "bundle",
+ DAG = "dag",
+ NONE = "none",
+}
+
+export type VersionIndicatorDisplayOption = VersionIndicatorDisplayOptions;
Review Comment:
thanks, I removed the redundant type alias and now using enum directly
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2777667098
##
airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py:
##
@@ -69,11 +69,15 @@ def _get_aggs_for_node(detail):
max_end_date = max(x["end_date"] for x in detail if x["end_date"])
except ValueError:
max_end_date = None
+
+dag_version_number = detail[0].get("dag_version_number")
Review Comment:
Thanks for catch this!
I fixed it :)
##
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -79,6 +81,11 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
);
const [showGantt, setShowGantt] =
useLocalStorage(`show_gantt-${dagId}`, false);
+ const [showVersionIndicatorMode, setShowVersionIndicatorMode] =
Review Comment:
Done👍
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2777666203
##
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx:
##
@@ -471,6 +493,64 @@ export const PanelButtons = ({
) : undefined}
)}
+{/* eslint-disable react/jsx-max-depth */}
Review Comment:
Agreed, extracted to a separate VersionIndicatorSelect component :)
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2777664606
##
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##
@@ -0,0 +1,123 @@
+/*!
+ * 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 } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiGitCommit } from "react-icons/fi";
+
+import { Tooltip } from "src/components/ui";
+
+type BundleVersionIndicatorProps = {
+ readonly bundleVersion: string | undefined;
+};
+
+export const BundleVersionIndicator = ({ bundleVersion }:
BundleVersionIndicatorProps) => {
+ const { t: translate } = useTranslation("components");
+
+ return (
+
+
+
+
+
+ );
+};
+
+type DagVersionIndicatorProps = {
+ readonly dagVersionNumber: number | undefined;
+ readonly orientation?: "horizontal" | "vertical";
+};
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: DagVersionIndicatorProps) => {
+ const isVertical = orientation === "vertical";
+
+ const containerStyles = {
Review Comment:
Thanks, Moved both CONTAINER_STYLES and CIRCLE_STYLES outside the component
as static constants 🙌
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2777663767
##
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -79,6 +81,11 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
);
const [showGantt, setShowGantt] =
useLocalStorage(`show_gantt-${dagId}`, false);
+ const [showVersionIndicatorMode, setShowVersionIndicatorMode] =
Review Comment:
Added a comment to clarify this is intentionally global.
The version indicator display preference applies across all Dags for a
consistent ux.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2777662943
##
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -79,6 +81,11 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
);
const [showGantt, setShowGantt] =
useLocalStorage(`show_gantt-${dagId}`, false);
+ const [showVersionIndicatorMode, setShowVersionIndicatorMode] =
Review Comment:
Done👍
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
guan404ming commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2773642696
##
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##
@@ -0,0 +1,123 @@
+/*!
+ * 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 } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiGitCommit } from "react-icons/fi";
+
+import { Tooltip } from "src/components/ui";
+
+type BundleVersionIndicatorProps = {
+ readonly bundleVersion: string | undefined;
+};
+
+export const BundleVersionIndicator = ({ bundleVersion }:
BundleVersionIndicatorProps) => {
+ const { t: translate } = useTranslation("components");
+
+ return (
+
+
+
+
+
+ );
+};
+
+type DagVersionIndicatorProps = {
+ readonly dagVersionNumber: number | undefined;
+ readonly orientation?: "horizontal" | "vertical";
+};
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: DagVersionIndicatorProps) => {
+ const isVertical = orientation === "vertical";
+
+ const containerStyles = {
+horizontal: {
+ height: 0.5,
+ left: "50%",
+ top: 0,
+ transform: "translate(-50%, -50%)",
+ width: 4.5,
+},
+vertical: {
+ height: 104,
+ left: -1.25,
+ top: -1.5,
+ width: 0.5,
+},
+ } as const;
+
+ const circleStyles = {
Review Comment:
ditto
##
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx:
##
@@ -471,6 +493,64 @@ export const PanelButtons = ({
) : undefined}
)}
+{/* eslint-disable react/jsx-max-depth */}
Review Comment:
Nit: Shall we try to make this a separated components like
`VersionIndicatorSelect` since this component is kind of over complex (for me)?
##
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -79,6 +81,11 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
);
const [showGantt, setShowGantt] =
useLocalStorage(`show_gantt-${dagId}`, false);
+ const [showVersionIndicatorMode, setShowVersionIndicatorMode] =
Review Comment:
Maybe we could use `version_indicator_display_mode-${dagId}` for
consistency, or document why this is intentionally global.
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,48 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export enum VersionIndicatorDisplayOptions {
+ ALL = "all",
+ BUNDLE = "bundle",
+ DAG = "dag",
+ NONE = "none",
+}
+
+export type VersionIndicatorDisplayOption = VersionIndicatorDisplayOptions;
Review Comment:
I think this might be kind of redundant. Consider removing the alias and
using the enum directly everywhere, or using a union type ("all" | "bundle" |
"dag" | "none") to decouple from the enum.
##
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##
@@ -0,0 +1,123 @@
+/*!
+ * 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
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3858582231 > Hi, this feature looks really cool. Could you please help resolve the conflicts? Thanks! Oops, I missed the notification earlier 😅 I've resolved the conflicts now. Thanks for pointing it out! -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3858579869 > Hi, this feature looks really cool. Could you please help resolve the conflicts? Thanks! Oops, I missed the notification earlier 😅 I've resolved the conflicts now. Thanks for pointing it out! -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
guan404ming commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3823175908 Hi, this feature looks really cool. Could you please help resolve the conflicts? Thanks! -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
bbovenzi commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3725630238 We'll need to rebase this after virtualizing the grid's vertical scroll. -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3685139005 Due to frequent merges from main, the commit history has become cluttered with conflict-resolution commits, so I’m going to continue the work in a new PR. -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2585974961 ## airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py: ## @@ -79,6 +79,9 @@ class GridRunsResponse(BaseModel): run_after: datetime state: DagRunState | None run_type: DagRunType +dag_version_number: int | None = None +bundle_version: str | None = None +has_mixed_versions: bool = False Review Comment: I understand the suggestion to reuse the existing APIs, and I explored the frontend-only approach, However, I have some concerns. One main issue is the Grid shows multiple Dag runs at the same time, and if we try to fetch the version information for each run individually using get_dag_run, the number of API calls quickly adds up. For example, if there are 10 runs displayed, we’d end up making 11 API calls in total—one for get_dag_details and one for each of the 10 runs. Could you let me know if what I found out is incorrect? -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on PR #53216: URL: https://github.com/apache/airflow/pull/53216#issuecomment-3615942888 Except for this part https://github.com/apache/airflow/pull/53216#discussion_r2585974961 I’ve marked the addressed comments as resolved and updated all the screenshots. @pierrejeambrun Could you review it once more? -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2586049589
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,44 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export const VersionIndicatorDisplayOptions = {
+ ALL: "all",
+ BUNDLE: "bundle",
+ DAG: "dag",
+ NONE: "none",
+} as const;
Review Comment:
wow good ;)
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2585974961 ## airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py: ## @@ -79,6 +79,9 @@ class GridRunsResponse(BaseModel): run_after: datetime state: DagRunState | None run_type: DagRunType +dag_version_number: int | None = None +bundle_version: str | None = None +has_mixed_versions: bool = False Review Comment: I understand the suggestion to reuse the existing APIs, and I explored the frontend-only approach, However, I have some concerns. One main issue is the N+1 query problem. The Grid shows multiple Dag runs at the same time, and if we try to fetch the version information for each run individually using get_dag_run, the number of API calls quickly adds up. For example, if there are 10 runs displayed, we’d end up making 11 API calls in total—one for get_dag_details and one for each of the 10 runs. Could you let me know if what I found out is incorrect? -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
pierrejeambrun commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2585924843
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,44 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export const VersionIndicatorDisplayOptions = {
+ ALL: "all",
+ BUNDLE: "bundle",
+ DAG: "dag",
+ NONE: "none",
+} as const;
Review Comment:
Probably using an enum or an explicit type (ts will detect mutation or
invalid assignments)
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
pierrejeambrun commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2585914897
##
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##
@@ -0,0 +1,111 @@
+/*!
+ * 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 } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiGitCommit } from "react-icons/fi";
+
+import { Tooltip } from "src/components/ui";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => {
+ const { t: translate } = useTranslation("components");
+
+ return (
+
+
+
+
+
+ );
+};
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+
+ {isVertical ? (
+<>
+
+
+
+
+
+
+ ) : (
+
+
+
Review Comment:
Sounds good
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2568031338
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,44 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export const VersionIndicatorDisplayOptions = {
+ ALL: "all",
+ BUNDLE: "bundle",
+ DAG: "dag",
+ NONE: "none",
+} as const;
Review Comment:
I used as const to preserve the literal types and ensure the object remains
readonly.
If there’s a better or alternative approach, please let me know :)
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2568031338
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,44 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export const VersionIndicatorDisplayOptions = {
+ ALL: "all",
+ BUNDLE: "bundle",
+ DAG: "dag",
+ NONE: "none",
+} as const;
Review Comment:
I used as const to preserve the literal types and ensure the object remains
readonly.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
choo121600 commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2567361143
##
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##
@@ -0,0 +1,111 @@
+/*!
+ * 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 } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiGitCommit } from "react-icons/fi";
+
+import { Tooltip } from "src/components/ui";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => {
+ const { t: translate } = useTranslation("components");
+
+ return (
+
+
+
+
+
+ );
+};
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+
+ {isVertical ? (
+<>
+
+
+
+
+
+
+ ) : (
+
+
+
Review Comment:
I share the same opinion as in [this
comment](https://github.com/apache/airflow/pull/53216#issuecomment-3497696807)
I hadn't added the commit yet because of the legend, but I'll go ahead and
add a commit with this design applied.
--
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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
pierrejeambrun commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2556395203 ## airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py: ## @@ -79,6 +79,9 @@ class GridRunsResponse(BaseModel): run_after: datetime state: DagRunState | None run_type: DagRunType +dag_version_number: int | None = None +bundle_version: str | None = None +has_mixed_versions: bool = False Review Comment: Same for this. You don't need to compute it. We have the `get_dag_run` query in the context of the grid, that returns `dag_versions` already, if the list is bigger than 1 we have mixed version. (only thing is that this list consider TaskInstanceHistory too, which might or might not be appropriate depending on how we want to visualize things) -- 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]
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
pierrejeambrun commented on code in PR #53216:
URL: https://github.com/apache/airflow/pull/53216#discussion_r2556369958
##
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts:
##
@@ -0,0 +1,44 @@
+/*!
+ * 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 { createListCollection } from "@chakra-ui/react";
+
+export const VersionIndicatorDisplayOptions = {
+ ALL: "all",
+ BUNDLE: "bundle",
+ DAG: "dag",
+ NONE: "none",
+} as const;
Review Comment:
why `as const` ?
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py:
##
@@ -79,6 +79,9 @@ class GridRunsResponse(BaseModel):
run_after: datetime
state: DagRunState | None
run_type: DagRunType
+dag_version_number: int | None = None
+bundle_version: str | None = None
+has_mixed_versions: bool = False
Review Comment:
Removing that will also simplify the backend code.
##
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx:
##
@@ -0,0 +1,111 @@
+/*!
+ * 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 } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { FiGitCommit } from "react-icons/fi";
+
+import { Tooltip } from "src/components/ui";
+
+export const BundleVersionIndicator = ({ bundleVersion }: { readonly
bundleVersion: string | null }) => {
+ const { t: translate } = useTranslation("components");
+
+ return (
+
+
+
+
+
+ );
+};
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+
+ {isVertical ? (
+<>
+
+
+
+
+
+
+ ) : (
+
+
+
Review Comment:
I don't think Horizontal `DagVersionIndicator` and `BundleVersionIndicator`
should use the same icon.
I would keep the `gitcommit` one for BundleVersions. And I would leave the
DagVersions as a contained 'chip'.
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py:
##
@@ -79,6 +79,9 @@ class GridRunsResponse(BaseModel):
run_after: datetime
state: DagRunState | None
run_type: DagRunType
+dag_version_number: int | None = None
+bundle_version: str | None = None
+has_mixed_versions: bool = False
Review Comment:
Same for this. You don't need to compute it. We have the `get_dag_run` query
in the context of the grid, that returns `dag_versions` already, if the list is
bigger than 1 we have mixed version. (only thing is that this list consider TIH
too, which might or might not be appropriate depending on how we want to
visualize things)
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py:
##
@@ -79,6 +79,9 @@ class GridRunsResponse(BaseModel):
run_after: datetime
state: DagRunState | None
run_type: DagRunType
+dag_version_number: int | None = None
Review Comment:
I don't think you need to pass this down, because the grid is calling
`get_dag_details` and the latest `dag_version` is already there in the payload.
So you might get it in the front-end from there. same for the bundle name
and bundle version.
##
airflo
Re: [PR] Feat: Add version change indicators for Dag and bundle versions in Grid view [airflow]
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 ;
}
+// 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 (
-
+
+{showVersionDivider ? (
+
+
+ {`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 }) => (
+
+
+
+);
+
+export const DagVersionIndicator = ({
+ dagVersionNumber,
+ orientation = "vertical",
+}: {
+ readonly dagVersionNumber: number | null;
+ readonly orientation?: "horizontal" | "vertical";
+}) => {
+ const isVertical = orientation === "vertical";
+
+ return (
+
+ {isVertical ? (
+<>
+
+
+ .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 }) =>
