You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/10/01 15:54:18 UTC
[GitHub] [airflow] bbovenzi opened a new pull request #18675: New Tree View
bbovenzi opened a new pull request #18675:
URL: https://github.com/apache/airflow/pull/18675
**Problem**
A tree diagram cannot properly represent a graph. This has led to a number of issues:
- users being confused seeing multiple leaves representing the same task
- displaying task groups has never really worked well #18009, #13242
- task groups have never had a collapse/expand functionality like graph view #16112
- The long tree names and branches easily leads to a overlapping elements #17717
- It is all built in a hard-to-follow d3 setup that makes maintaining the view hard
**Solution**
- Remove the "tree" part of tree view. Show runs and tasks but leave dependency lines to the graph view
- Allow a user to better abstract away parts of their DAGs with Task Groups and simplify their workflows
- Build in a modern technology that we can more easily maintain
**Screenshots**
<img width="1192" alt="Screen Shot 2021-10-01 at 5 16 29 PM" src="https://user-images.githubusercontent.com/4600967/135648242-717acc77-a5ab-4bd5-a560-e3199acf74a0.png">
<img width="788" alt="Screen Shot 2021-10-01 at 5 15 39 PM" src="https://user-images.githubusercontent.com/4600967/135648305-5514f9ce-f94b-4744-b5a6-279b039e2c53.png">
<img width="325" alt="Screen Shot 2021-10-01 at 5 16 48 PM" src="https://user-images.githubusercontent.com/4600967/135648325-9ae934c9-361a-4ee9-8186-8914e6064e76.png">
**Features and Benefits**
- Show durations of dag runs to quickly see performance changes #8931
- Task groups have summaries and can expand/collapse just like the graph view
- Grid lines and hover effects to see which task you are inspecting
- Built in React to more easily maintain and can bring over to an eventual new Airflow UI in AIP-38
- Ready to add new features in the roadmap like Dag Versioning or Task Mapping
Subsequent PRs:
- Add testing of react components in the current UI `/airflow/www`
- Filter over the state legend to highlight tasks and dag runs like graph view
- More elegantly handle outliers in the Dag Run Duration chart (if it normally takes 5 minutes but then a run takes 5 hours the graph won't look great)
- Update the Task Instance and Dag Run Action Modals too
- More dynamic scrolling around the 100s of dag runs instead of the clunky "Update" button
- Improve render times with Lazy Loading #13066
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-937906703
I made some changes from everyone's feedback:
- Updated the hover/group colors to gray instead of blue.
- Extended the vertical date ticks to span the whole dag run duration chart
But two things I'll push back on:
- I think the play icon is still best to indicate a manual run because we use it for the trigger button.
- We already use gray colors to indicate children of Task Groups and on hover. Adding it to even/odd rows would be too noisy.
<img width="677" alt="Screen Shot 2021-10-07 at 10 23 11 AM" src="https://user-images.githubusercontent.com/4600967/136414969-3f69fdd6-2d6c-4ad1-b05d-c72f323ed3c4.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r725086052
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
+ return {
+ 'id': task_group.task_id,
+ 'instances': [wwwutils.encode_ti(ti) for ti in tis if ti.task_id == task_group.task_id],
+ 'label': task_group.label,
+ 'extra_links': dag.get_task(task_group.task_id).extra_links,
+ }
+
+ children = [task_group_to_tree(child, dag, dag_runs, tis) for child in task_group.children.values()]
+
+ def get_summary(dag_run, children):
+ priority = [
+ 'failed',
+ 'upstream_failed',
+ 'up_for_retry',
+ 'up_for_reschedule',
+ 'queued',
+ 'scheduled',
+ 'deferred',
+ 'sensing',
+ 'running',
+ 'shutdown',
+ 'restarting',
+ 'removed',
+ 'no_status',
+ 'success',
+ 'skipped',
+ ]
+
+ child_instances = [child['instances'] for child in children if child.get('instances')]
Review comment:
Will fix this and some other reviews, also see if testing can be possible
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r725692558
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
Review comment:
I can't think of a better name 🤔 Do you have suggestion on the name? I used `task_object` but it didn't go down well with me.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032426836
> Users are forced to check the Graph Tab at the same time in a new browser tab
Brent is also working on making the graph view visible in the new grid view in a ... float in. So you could focus a dag run and you see, say 5 runs in the grid on the left, and what was the graph view before is now visible in the main/RHS of the page.
Anyway -- wait and see, and if that doesn't solve the need we can add the relations on hover.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r745710768
##########
File path: airflow/www/package.json
##########
@@ -56,19 +60,29 @@
"webpack-manifest-plugin": "^2.2.0"
},
"dependencies": {
+ "@chakra-ui/react": "^1.6.6",
+ "@emotion/cache": "^11.4.0",
+ "@emotion/react": "^11.4.1",
+ "@emotion/styled": "^11",
"bootstrap-3-typeahead": "^4.0.2",
+ "camelcase-keys": "^7.0.0",
"codemirror": "^5.59.1",
"d3": "^3.4.4",
"d3-shape": "^2.1.0",
"d3-tip": "^0.9.1",
"dagre-d3": "^0.6.4",
"datatables.net": "^1.10.23",
"datatables.net-bs": "^1.10.23",
+ "dayjs": "^1.10.6",
"eonasdan-bootstrap-datetimepicker": "^4.17.47",
+ "framer-motion": "^4",
"jquery": ">=3.5.0",
"jshint": "^2.12.0",
"moment-timezone": "^0.5.28",
"nvd3": "^1.8.6",
+ "react": "^17.0.2",
+ "react-dom": "^17.0.2",
+ "react-icons": "^4.2.0",
Review comment:
Oh, we'll need to include the license for _anything_ that webpack pulls in and thus we ship directly as part of the our artefacts. That is... tedius.
@bbovenzi Something like https://github.com/codepunkt/webpack-license-plugin might be what we need here - us trying to maintain it manually is just never going to work.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r745710768
##########
File path: airflow/www/package.json
##########
@@ -56,19 +60,29 @@
"webpack-manifest-plugin": "^2.2.0"
},
"dependencies": {
+ "@chakra-ui/react": "^1.6.6",
+ "@emotion/cache": "^11.4.0",
+ "@emotion/react": "^11.4.1",
+ "@emotion/styled": "^11",
"bootstrap-3-typeahead": "^4.0.2",
+ "camelcase-keys": "^7.0.0",
"codemirror": "^5.59.1",
"d3": "^3.4.4",
"d3-shape": "^2.1.0",
"d3-tip": "^0.9.1",
"dagre-d3": "^0.6.4",
"datatables.net": "^1.10.23",
"datatables.net-bs": "^1.10.23",
+ "dayjs": "^1.10.6",
"eonasdan-bootstrap-datetimepicker": "^4.17.47",
+ "framer-motion": "^4",
"jquery": ">=3.5.0",
"jshint": "^2.12.0",
"moment-timezone": "^0.5.28",
"nvd3": "^1.8.6",
+ "react": "^17.0.2",
+ "react-dom": "^17.0.2",
+ "react-icons": "^4.2.0",
Review comment:
Oh, we'll need to include the license for _anything_ that webpack pulls in. That is... sucky.
@bbovenzi Something like https://github.com/codepunkt/webpack-license-plugin might be what we need here - us trying to maintain it manually is just never going to work.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1030637110
Idea: @pingzh @bbovenzi - maybe it would be a possibility to list the upstream/downstream tasks in the tooltip when hovering over the task name?
I think that might serve the purpose @pingzh asks for (and it is actually something that @cploonker idea was pretty much about when you look closer). Maybe that is a simple way to get this without complicating the page too much.
I do not think we really need to see the whole graph in this view, but if we can just see the upstream/downstream when hovering over task name, maybe that could help?
I actually think seeing whole graph here is far too much but ability to see the direct upstream/downstream for particular task when you focus on it?
This might be tricky in big graphs but maybe some dynamic behaviour can be added to reveal full list when needed when it is long?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032906437
> Displaying the list and the relationships in different visualization creates clutter on the screen. Mentally one has to go thru both and combine them together in one's head. I worked at Facebook earlier and FB already displays both in a single visualization without any repetition of nodes. So it is definitely possible. If you have a friend at FB talk to them. I don't think it is worth your time to build incremental improvement to current solution. i would just leave it as is and wait for someone to come along and implement something on the lines of what FB has.
I think it does create clutter if visible all the time. But if it is optional, it has great value for the user, who does not have to open two tabs to map those two (as @pingzh mentioned). IMHO it shoudl be optional by making it hidden by default with a small "reveal/hide graph" button with animated slide-in from the right to bring the graph window - this would be just great.
This way in most cases there will be no clutter, but you will be able to bring the graph as needed. Good job @bbovenzi !
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748373404
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I copied this from `NOTICE`. Is there a way to interpolate it there too?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-966962082
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-967777120
very cool
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032906437
> Displaying the list and the relationships in different visualization creates clutter on the screen. Mentally one has to go thru both and combine them together in one's head. I worked at Facebook earlier and FB already displays both in a single visualization without any repetition of nodes. So it is definitely possible. If you have a friend at FB talk to them. I don't think it is worth your time to build incremental improvement to current solution. i would just leave it as is and wait for someone to come along and implement something on the lines of what FB has.
I think it does create clutter if visible all the time. But if it is optional, it has great value for the user, who does not have to open two tabs to map those two (as @pingzh mentioned). IMHO it shoudl be optional by making it hidden by default with a small "reveal/hide graph" button with animated slide-in from the right to bring the graph window would be just great.
This way in most cases there will be no clutter, but you will be able to bring the graph as needed. Good job @bbovenzi !
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pingzh commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
pingzh commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032032585
@potiuk yep, adding the upstream/downstream to this new Tree view also works well. I think it is very important for users to easily see upstream/downstream in the Tree view when they do operations for their task runs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r725705965
##########
File path: airflow/www/static/js/tree/useTreeData.js
##########
@@ -0,0 +1,79 @@
+/*!
+ * 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.
+ */
+
+/* global treeData, localStorage, fetch, autoRefreshInterval */
+
+import { useState, useEffect, useCallback } from 'react';
+import { useDisclosure } from '@chakra-ui/react';
+import camelcaseKeys from 'camelcase-keys';
+
+import getMetaValue from '../meta_value';
+
+// dagId comes from dag.html
+const dagId = getMetaValue('dag_id');
+const treeDataUrl = getMetaValue('tree_data');
+const numRuns = getMetaValue('num_runs');
+const urlRoot = getMetaValue('root');
+const isPaused = getMetaValue('is_paused');
+
+const areActiveRuns = (runs) => runs.filter((run) => ['queued', 'running', 'scheduled'].includes(run.state)).length > 0;
+
+const useTreeData = () => {
+ const [data, setData] = useState(camelcaseKeys(JSON.parse(treeData), { deep: true }));
+ const defaultIsOpen = isPaused !== 'True' && (JSON.parse(localStorage.getItem('disableAutoRefresh')) || areActiveRuns(data.dagRuns));
+ const { isOpen, onToggle } = useDisclosure({ defaultIsOpen });
+
+ const handleRefresh = useCallback(async () => {
+ const resp = await fetch(`${treeDataUrl}?dag_id=${dagId}&num_runs=${numRuns}&root=${urlRoot}`);
+ let newData = await resp.json();
+ newData = camelcaseKeys(newData, { deep: true });
+ if (JSON.stringify(newData) !== JSON.stringify(data)) {
Review comment:
Are we sure that all elements will always have the same order?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-967702562
Wohoo... great work @bbovenzi 🎉
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r720781081
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
+ return {
+ 'id': task_group.task_id,
+ 'instances': [wwwutils.encode_ti(ti) for ti in tis if ti.task_id == task_group.task_id],
+ 'label': task_group.label,
+ 'extra_links': dag.get_task(task_group.task_id).extra_links,
+ }
+
+ children = [task_group_to_tree(child, dag, dag_runs, tis) for child in task_group.children.values()]
+
+ def get_summary(dag_run, children):
+ priority = [
+ 'failed',
+ 'upstream_failed',
+ 'up_for_retry',
+ 'up_for_reschedule',
+ 'queued',
+ 'scheduled',
+ 'deferred',
+ 'sensing',
+ 'running',
+ 'shutdown',
+ 'restarting',
+ 'removed',
+ 'no_status',
+ 'success',
+ 'skipped',
+ ]
+
+ child_instances = [child['instances'] for child in children if child.get('instances')]
Review comment:
```suggestion
child_instances = [child['instances'] for child in children if 'instances' in child]
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r720780406
##########
File path: airflow/www/utils.py
##########
@@ -41,6 +43,47 @@
from airflow.www.widgets import AirflowDateTimePickerWidget
+def datetime_to_string(value: Optional[DateTime]) -> Optional[str]:
Review comment:
I think this can be just (the stdlib) `datetime.datetime` and not restricted to `pendulum`. Also there's a similar function in `views.py` so we should refactor that to use this function as 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-933282239
Love it, well done @bbovenzi .
Early feedback:
Not sure if this is already covered but Changing the background with a darker or lighter shade on hovering a task would be great, especially if there are tons of runs.
Example:
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r733109746
##########
File path: airflow/www/static/js/tree/Tree.jsx
##########
@@ -0,0 +1,202 @@
+/*!
+ * 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.
+ */
+
+/* global localStorage */
+
+import React, { useRef, useState } from 'react';
+import {
+ Flex,
+ Box,
+ Text,
+ useDisclosure,
+ Collapse,
+ Switch,
+ FormControl,
+ FormLabel,
+ Spinner,
+} from '@chakra-ui/react';
+import { FiChevronUp, FiChevronDown } from 'react-icons/fi';
+
+import useTreeData from './useTreeData';
+import StatusBox from './StatusBox';
+import DagRunBar from './DagRunBar';
+import getMetaValue from '../meta_value';
+import { getDuration, formatDuration } from './utils';
+
+// dagId comes from dag.html
+const dagId = getMetaValue('dag_id');
+
+const DurationTick = ({ children, ...rest }) => (
+ <Text fontSize={10} color="gray.400" position="absolute" left="calc(100% + 6px)" whiteSpace="nowrap" {...rest}>
+ {children}
+ </Text>
+);
+
+const Tree = () => {
+ const containerRef = useRef();
+ const { data, refreshOn, onToggleRefresh } = useTreeData();
+ const { groups = [], dagRuns = [] } = data;
+ const durations = [];
+ const runs = dagRuns.map((dagRun) => {
+ const duration = getDuration(dagRun.startDate, dagRun.endDate);
+ durations.push(duration);
+ return {
+ ...dagRun,
+ duration,
+ };
+ });
+
+ // calculate dag run bar heights relative to max
+ const max = Math.max.apply(null, durations);
+
+ const renderGroup = (prevGroup, level) => prevGroup.children.map((group) => (
+ <TaskGroup key={group.id} group={group} level={level} prevGroup={prevGroup.id} />
+ ));
+
+ const TaskGroup = ({ prevGroup, group, level }) => {
+ const isGroup = !!group.children;
+
+ const storageKey = `${dagId}-open-groups`;
+ const openGroups = JSON.parse(localStorage.getItem(storageKey)) || [];
+ const isGroupId = openGroups.some((g) => g === group.id);
+
+ const onOpen = () => {
+ localStorage.setItem(storageKey, JSON.stringify([...openGroups, group.id]));
+ };
+ const onClose = () => {
+ localStorage.setItem(storageKey, JSON.stringify(openGroups.filter((g) => g !== group.id)));
+ };
+
+ const { isOpen, onToggle } = useDisclosure({ defaultIsOpen: isGroupId, onClose, onOpen });
+ const [isHover, setHover] = useState(false);
+ // Show only the immediate task id
+ const taskName = group.id.replace(`${prevGroup}.`, '');
+
+ return (
+ <Box backgroundColor={`rgba(203, 213, 224, ${0.25 * level})`}>
+ <Flex
+ justifyContent="space-between"
+ alignItems="center"
+ width="100%"
+ py="1px"
+ borderBottomWidth={1}
+ borderBottomColor={level > 1 ? 'white' : 'gray.200'}
+ backgroundColor={isHover && 'rgba(113, 128, 150, 0.1)'}
+ onMouseEnter={() => setHover(true)}
+ onMouseLeave={() => setHover(false)}
+ >
+ <Flex
+ as={isGroup ? 'button' : 'div'}
+ onClick={() => isGroup && onToggle()}
+ color={(level > 4 || (isHover && level > 3)) && 'white'}
Review comment:
Wondering if you could avoid the `isHover` state with something like this?
```suggestion
role="group"
_hover={{ backgroundColor: 'rgba(113, 128, 150, 0.1'}}
>
<Flex
as={isGroup ? 'button' : 'div'}
onClick={() => isGroup && onToggle()}
color={level > 4 ? 'white' : undefined}
_groupHover={level > 3 && {
color: 'white',
}}
```
##########
File path: airflow/www/static/js/tree/DagRunTooltip.jsx
##########
@@ -0,0 +1,102 @@
+/*!
+ * 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.
+ */
+
+/* global moment */
+
+import React from 'react';
+import { Box, Text } from '@chakra-ui/react';
+import { MdPlayArrow } from 'react-icons/md';
+
+import { formatDateTime } from '../datetime_utils';
+import { formatDuration } from './utils';
+
+const DagRunTooltip = ({
+ dagRun: {
+ state, runId, duration, dataIntervalStart, dataIntervalEnd, startDate, endDate, runType,
+ },
+}) => (
+ <Box fontSize={12}>
+ <br />
Review comment:
Are the `<br />`s at the top and bottom necessary? If you do want extra spacing, maybe add `py={4}` to the parent `Box` instead?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-963741391
We're now testing our react components. Setting up jest in `airflow/www` will also allow us to write tests for our current js files too.
<img width="661" alt="Screen Shot 2021-11-08 at 7 54 50 PM" src="https://user-images.githubusercontent.com/4600967/140847344-d0f24e63-53b1-436b-b6dd-6d9e4cfe8b47.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r727529032
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
Review comment:
`task_item_or_group`? (And we can assign it to new names after the `isinstance()` check)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1013903135
> Is it possible to include dependency lines into this view using something like https://observablehq.com/@erikbrinkman/d3-dag-topological 
Good idea! It's definitely worth exploring sometime.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] cploonker commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
cploonker commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032841800
Displaying the list and the relationships in different visualization creates clutter on the screen. Mentally one has to go thru both and combine them together in one's head.
I worked at Facebook earlier and FB already displays both in a single visualization without any repetition of nodes. So it is definitely possible. If you have a friend at FB talk to them.
I don't think it is worth your time to build incremental improvement to current solution. i would just leave it as is and wait for someone to come along and implement something on the lines of what FB has.
I did talk to a friend at FB to get their implementation open sourced. Unfortunately due to time constraints in every companies it is very difficult to get such a project prioritized. That is why i started searching on how others are doing and found [Erik's library mentioned above](https://observablehq.com/@erikbrinkman/d3-dag-topological). All we need is to add the grid which shows run status to the right side of that visualization which already renders dependencies and kind of vertical list in a single visualization. Eric is also very cooperative in terms of answering any questions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-967799567
Yep! Cool!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] akotlar commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
akotlar commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-974877095
This looks much, much better. Thanks so much
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r726216648
##########
File path: airflow/www/static/js/tree/useTreeData.js
##########
@@ -0,0 +1,79 @@
+/*!
+ * 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.
+ */
+
+/* global treeData, localStorage, fetch, autoRefreshInterval */
+
+import { useState, useEffect, useCallback } from 'react';
+import { useDisclosure } from '@chakra-ui/react';
+import camelcaseKeys from 'camelcase-keys';
+
+import getMetaValue from '../meta_value';
+
+// dagId comes from dag.html
+const dagId = getMetaValue('dag_id');
+const treeDataUrl = getMetaValue('tree_data');
+const numRuns = getMetaValue('num_runs');
+const urlRoot = getMetaValue('root');
+const isPaused = getMetaValue('is_paused');
+
+const areActiveRuns = (runs) => runs.filter((run) => ['queued', 'running', 'scheduled'].includes(run.state)).length > 0;
+
+const useTreeData = () => {
+ const [data, setData] = useState(camelcaseKeys(JSON.parse(treeData), { deep: true }));
+ const defaultIsOpen = isPaused !== 'True' && (JSON.parse(localStorage.getItem('disableAutoRefresh')) || areActiveRuns(data.dagRuns));
+ const { isOpen, onToggle } = useDisclosure({ defaultIsOpen });
+
+ const handleRefresh = useCallback(async () => {
+ const resp = await fetch(`${treeDataUrl}?dag_id=${dagId}&num_runs=${numRuns}&root=${urlRoot}`);
+ let newData = await resp.json();
+ newData = camelcaseKeys(newData, { deep: true });
+ if (JSON.stringify(newData) !== JSON.stringify(data)) {
Review comment:
I guess not, but if the order does change its not a bad idea to rerender the tree anyway.
I'm open to suggestions on how we should order tasks in the webserver.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r720780836
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
Review comment:
The variant should probably not be named `task_group` 😛
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032806638
I was waiting to reply until I had, at least, a rough draft to show off. But as Ash said, I think the ultimate solution is to show both the tree and graph at the same time. This will make it far easier to switch between different dag runs and task instances.
<img width="865" alt="Screen Shot 2022-02-08 at 11 20 36 AM" src="https://user-images.githubusercontent.com/4600967/153029744-13652fd8-74ab-466d-a550-8d9bf5790649.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r727529032
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
Review comment:
`task_item_or_group`? (And we can assign it to new names after the `isinstance()` check)
##########
File path: airflow/www/static/js/tree/useTreeData.js
##########
@@ -0,0 +1,79 @@
+/*!
+ * 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.
+ */
+
+/* global treeData, localStorage, fetch, autoRefreshInterval */
+
+import { useState, useEffect, useCallback } from 'react';
+import { useDisclosure } from '@chakra-ui/react';
+import camelcaseKeys from 'camelcase-keys';
+
+import getMetaValue from '../meta_value';
+
+// dagId comes from dag.html
+const dagId = getMetaValue('dag_id');
+const treeDataUrl = getMetaValue('tree_data');
+const numRuns = getMetaValue('num_runs');
+const urlRoot = getMetaValue('root');
+const isPaused = getMetaValue('is_paused');
+
+const areActiveRuns = (runs) => runs.filter((run) => ['queued', 'running', 'scheduled'].includes(run.state)).length > 0;
+
+const useTreeData = () => {
+ const [data, setData] = useState(camelcaseKeys(JSON.parse(treeData), { deep: true }));
+ const defaultIsOpen = isPaused !== 'True' && (JSON.parse(localStorage.getItem('disableAutoRefresh')) || areActiveRuns(data.dagRuns));
+ const { isOpen, onToggle } = useDisclosure({ defaultIsOpen });
+
+ const handleRefresh = useCallback(async () => {
+ const resp = await fetch(`${treeDataUrl}?dag_id=${dagId}&num_runs=${numRuns}&root=${urlRoot}`);
+ let newData = await resp.json();
+ newData = camelcaseKeys(newData, { deep: true });
+ if (JSON.stringify(newData) !== JSON.stringify(data)) {
Review comment:
Python dicts are insertion-ordered (from 3.6 up at least), so this should be mostly stable. The most likely source of issues would be the first refresh, because the initial `treeData` is from the HTML, but subsequent runs use the JSON-based view, and those are more likely to have inconsistencies if we're not careful. I kind of wonder if we should get rid of the initial `treeData` and always render from the JSON view.
The other significant possibility of inconsistencies is SQL natural ordering, but those can be caught incrementally if needed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-933033822
I ❤️ where it goes. I do not know too well the UI part, and I need to look more at it but it looks so much more usable than the old tree view.
I have not looked at the code but I have a few small "UX" comments and improvement proposals:
* I'd add is to have slightly different shade of gray background for alternating rows of the same type (same level). This make whole world of difference in "grid-like" view - you immediately see which row value you actually look at if you are looking at far-left column. (this will work without hovering the mouse).
* I love the duration though indeed without handling outliers it will be only useful in "non-interesting" cases - where all durations are similiar, so I think handling outliers is absolutely needed.,
* (might be more difficult) I think we also need some vertical lines for the time spans - nice to have but likely the grid lines should follow the same decision rules as for the time labels.
* I think "play" on manual runs is not the best choice. The "play" suggests that it is an "actionable" button. I'd rather use a small "hand" there (akin to https://icon-library.com/icon/icon-manual-2.html), I thnk much more "static" meaning even if it's not the same as "trigger" button.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-937906703
I made some changes from everyone's feedback:
- Updated the hover/group colors to gray instead of blue.
- Extended the vertical date ticks to span the whole dag run duration chart
But two things I'll push back on:
- I think the play icon is still best to indicate a manual run because we use it for the trigger button.
- We already use gray colors to indicate children of Task Groups and on hover. Adding it to even/odd rows would be too noisy.
<img width="677" alt="Screen Shot 2021-10-07 at 10 23 11 AM" src="https://user-images.githubusercontent.com/4600967/136414969-3f69fdd6-2d6c-4ad1-b05d-c72f323ed3c4.png">
<img width="313" alt="Screen Shot 2021-10-07 at 10 36 21 AM" src="https://user-images.githubusercontent.com/4600967/136417304-605a2edc-2918-49c4-9eda-089ea2c8c754.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] cploonker commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
cploonker commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1013594704
Is it possible to include dependency lines into this view using something like https://observablehq.com/@erikbrinkman/d3-dag-topological
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] akotlar commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
akotlar commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-974877095
This looks much, much better. Thanks so much
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032917376
> That is why i started searching on how others are doing and found [Erik's library mentioned above](https://observablehq.com/@erikbrinkman/d3-dag-topological). All we need is to add the grid which shows run status to the right side of that visualization which already renders dependencies and kind of vertical list in a single visualization. Eric is also very cooperative in terms of answering any questions.
I do not like this solution because it only shows relations wilth lines which might go up and down arbitrary number of rows (including long lines that will stretch far beyond the screen in big graphs). The big benefit of the solution proposed by @bbovenzi is that you see the direct upstream/downstream task names immediately next to the task in the graph, even if in the grid they will be many, many rows away.
Tracking such relations will be next to impossible if we add lines to the grid.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-937906703
I made some changes from everyone's feedback:
- Updated the hover/group colors to gray instead of blue.
- Extended the vertical date ticks to span the whole dag run duration chart
But two things I'll push back on:
- I think the play icon is still best to indicate a manual run because it matches the trigger run button.
- We already use gray colors to indicate children of Task Groups and on hover. Adding it to even/odd rows would be too noisy.
<img width="677" alt="Screen Shot 2021-10-07 at 10 23 11 AM" src="https://user-images.githubusercontent.com/4600967/136414969-3f69fdd6-2d6c-4ad1-b05d-c72f323ed3c4.png">
<img width="313" alt="Screen Shot 2021-10-07 at 10 36 21 AM" src="https://user-images.githubusercontent.com/4600967/136417304-605a2edc-2918-49c4-9eda-089ea2c8c754.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r725705167
##########
File path: airflow/www/static/js/tree/DagRunBar.jsx
##########
@@ -0,0 +1,84 @@
+/*!
+ * 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.
+ */
+
+/* global stateColors, moment */
+
+import React from 'react';
+import {
+ Flex,
+ Box,
+ Tooltip,
+ Text,
+ VStack,
+} from '@chakra-ui/react';
+import { MdPlayArrow } from 'react-icons/md';
+
+import DagRunTooltip from './DagRunTooltip';
+import { callModalDag } from '../dag';
+
+const DagRunBar = ({
+ run, max, index, totalRuns, containerRef,
+}) => (
+ <Box position="relative">
+ <Flex
+ height="100px"
+ alignItems="flex-end"
+ pb={1}
+ px={1}
+ zIndex={1}
+ onClick={() => {
+ callModalDag({ execution_date: run.executionDate, dagId: run.dagId });
+ }}
+ >
+ <Tooltip
+ label={<DagRunTooltip dagRun={run} />}
+ hasArrow
+ portalProps={{ containerRef }}
+ placement="top"
+ openDelay={10}
+ >
+ <Flex
+ width="10px"
+ height={`${(run.duration / max) * 100}px`}
+ minHeight="10px"
+ backgroundColor={stateColors[run.state]}
+ borderRadius={2}
+ cursor="pointer"
+ pb="1px"
+ direction="column"
+ justifyContent="flex-end"
+ alignItems="center"
+ zIndex={2}
+ >
+ {run.runType === 'manual' && <MdPlayArrow size="8px" color="white" />}
+ </Flex>
+ </Tooltip>
+ </Flex>
+ {index < totalRuns - 4 && index % 10 === 0 && (
+ <VStack position="absolute" top="0" left="-23px" spacing={0}>
+ <Text fontSize={10} color="gray.400" whiteSpace="nowrap" transform="rotate(-30deg) translateX(32px)" mt="-23px !important">
+ {moment.utc(run.executionDate).format('MMM DD, HH:mm')}
Review comment:
Why do we sometimes use the `moment.js` library and other times the day.js library?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r747762208
##########
File path: airflow/www/package.json
##########
@@ -56,19 +60,29 @@
"webpack-manifest-plugin": "^2.2.0"
},
"dependencies": {
+ "@chakra-ui/react": "^1.6.6",
+ "@emotion/cache": "^11.4.0",
+ "@emotion/react": "^11.4.1",
+ "@emotion/styled": "^11",
"bootstrap-3-typeahead": "^4.0.2",
+ "camelcase-keys": "^7.0.0",
"codemirror": "^5.59.1",
"d3": "^3.4.4",
"d3-shape": "^2.1.0",
"d3-tip": "^0.9.1",
"dagre-d3": "^0.6.4",
"datatables.net": "^1.10.23",
"datatables.net-bs": "^1.10.23",
+ "dayjs": "^1.10.6",
"eonasdan-bootstrap-datetimepicker": "^4.17.47",
+ "framer-motion": "^4",
"jquery": ">=3.5.0",
"jshint": "^2.12.0",
"moment-timezone": "^0.5.28",
"nvd3": "^1.8.6",
+ "react": "^17.0.2",
+ "react-dom": "^17.0.2",
+ "react-icons": "^4.2.0",
Review comment:
I updated the PR to use the above library in our `webpack.config.js` to auto-generate a `licenses/LICENSES-ui.txt` file when running `yarn run build`. The file is gitignored but will be included in any builds during `setup.py`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748373404
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I copied this from `NOTICE`. Is there a way to interpolate it there too? or is `2016-present` allowed?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi merged pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi merged pull request #18675:
URL: https://github.com/apache/airflow/pull/18675
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748385017
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I believe it's not allowed.
My interpretation is that you have to state what was the year your contributions were made and it should not be
a) future, because for example it would limit you so that you could not change licencing scheme until it expires
b) `present` verbatim - because it is meaningless in the context of "writer". The `present` only makes sense in the context of "reader" because you do not keep in the file the date when "present" was put. However, the licence file needs the context of "writer", so you need to specify when was the time the licence was last "expected" to be granted by those who have the rights to provide the licence.
We usually perform a bulk update of all licence dates at the beginning of the year. This is also clear expression of intent "yes we want to licence our code under the ASF licence also in 2022". So do not worry about it now.
We usually update all the (C) notices
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pingzh commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
pingzh commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1030375697
Hi @bbovenzi great work, this looks awesome. i am wondering if we can still have the original tree view tab but having this new tree view as a new table view or something else. since with the new table view, it losses the ability to view downstream or upstream of a task. Users are forced to check the Graph Tab at the same time in a new browser tab. For large DAGs, it will be very inconvenient.
Please let me know your thoughts. thanks.
---
Another quick question, is it possible to add a highlight box around the tis when I hover over a task_id. For example, if i hover over `task_3`, all tis with the `task_3` will be highlighted so that it is very easy for users to identify them given that there could be lots of squares with the same color.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1030637110
Idea: @pingzh @bbovenzi - maybe it would be a possibility to list the upstream/downstream tasks in the tooltip when hovering over the task name?
I think that might serve the purpose @pingzh asks for (and it is actually something that @cploonker idea was pretty much about when you look closer). Maybe that is a simple way to get this without complicating the page too much.
I do not think we really need to see the whole graph in this view, but if we can just see the upstream/downstream when hovering over task name, maybe that could help?
I actually think seeing whole graph here is far too much but ability to see the direct upstream/downstream for particular task when you focus on it?
This might be tricky in big graphs but maybe some dynamic behaviour can be added to reveal that when needed?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032806638
I was waiting to reply until I had at least a rough draft to show off. But as Ash said, I think the ultimate solution is to show both the tree and graph at the same time. This will make it far easier to switch between different dag runs and task instances.
<img width="865" alt="Screen Shot 2022-02-08 at 11 20 36 AM" src="https://user-images.githubusercontent.com/4600967/153029744-13652fd8-74ab-466d-a550-8d9bf5790649.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r725705471
##########
File path: airflow/www/static/js/tree/InstanceTooltip.jsx
##########
@@ -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.
+ */
+
+/* global moment */
+
+import React from 'react';
+import { Box, Text } from '@chakra-ui/react';
+import dayjs from 'dayjs';
+import dayjsDuration from 'dayjs/plugin/duration';
+
+import { formatDateTime } from '../datetime_utils';
+
+dayjs.extend(dayjsDuration);
+
+const getDuration = (startDate, endDate) => (
+ dayjs(endDate || undefined).diff(startDate || undefined)
+);
+
+const InstanceTooltip = ({
+ group,
+ instance: {
+ duration, operator, startDate, endDate, state, taskId, runId,
+ },
+}) => {
+ const isGroup = !!group.children;
+
+ const groupSummary = [];
+
+ if (isGroup) {
+ const numMap = new Map([
+ ['success', 0],
+ ['failed', 0],
+ ['upstream_failed', 0],
+ ['up_for_retry', 0],
+ ['up_for_reschedule', 0],
+ ['running', 0],
+ ['deferred', 0],
+ ['queued', 0],
+ ['scheduled', 0],
+ ['skipped', 0],
+ ['no_status', 0],
+ ]);
+ group.children.forEach((child) => {
+ const taskInstance = child.instances.find((ti) => ti.runId === runId);
+ if (taskInstance) {
+ const stateKey = taskInstance.state == null ? 'no_status' : taskInstance.state;
+ if (numMap.has(stateKey)) numMap.set(stateKey, numMap.get(stateKey) + 1);
+ }
+ });
+ numMap.forEach((key, val) => {
+ if (key > 0) {
+ groupSummary.push(
+ // eslint-disable-next-line react/no-array-index-key
+ <Text key={val} ml="10px">
+ {val}
+ {': '}
+ {key}
+ </Text>,
+ );
+ }
+ });
+ }
+
+ const instanceDuration = duration || getDuration(startDate, endDate);
+
+ const taskIdTitle = isGroup ? 'Task Group Id: ' : 'Task Id: ';
+
+ return (
+ <Box fontSize={12}>
+ <br />
+ <Text>
+ <Text as="strong">Status:</Text>
+ {' '}
+ {state || 'no status'}
+ </Text>
+ {isGroup && (
+ <>
+ <br />
+ <Text as="strong">Group Summary</Text>
+ {groupSummary}
+ </>
+ )}
+ <br />
+ <Text>
+ {taskIdTitle}
+ {taskId}
+ </Text>
+ <Text whiteSpace="nowrap">
+ Run Id:
+ {' '}
+ {runId}
+ </Text>
+ {operator && (
+ <Text>
+ Operator:
+ {' '}
+ {operator}
+ </Text>
+ )}
+ <Text>
+ Duration:
+ {' '}
+ {dayjs.duration(instanceDuration).days() > 0 && `${dayjs.duration(instanceDuration).days()}d `}
Review comment:
Should we extract this fragment to `formatDuration` function similar to `formatDateTime`?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r745487589
##########
File path: airflow/www/package.json
##########
@@ -56,19 +60,29 @@
"webpack-manifest-plugin": "^2.2.0"
},
"dependencies": {
+ "@chakra-ui/react": "^1.6.6",
+ "@emotion/cache": "^11.4.0",
+ "@emotion/react": "^11.4.1",
+ "@emotion/styled": "^11",
"bootstrap-3-typeahead": "^4.0.2",
+ "camelcase-keys": "^7.0.0",
"codemirror": "^5.59.1",
"d3": "^3.4.4",
"d3-shape": "^2.1.0",
"d3-tip": "^0.9.1",
"dagre-d3": "^0.6.4",
"datatables.net": "^1.10.23",
"datatables.net-bs": "^1.10.23",
+ "dayjs": "^1.10.6",
"eonasdan-bootstrap-datetimepicker": "^4.17.47",
+ "framer-motion": "^4",
"jquery": ">=3.5.0",
"jshint": "^2.12.0",
"moment-timezone": "^0.5.28",
"nvd3": "^1.8.6",
+ "react": "^17.0.2",
+ "react-dom": "^17.0.2",
+ "react-icons": "^4.2.0",
Review comment:
I checked these libs and they are MIT, BSD (2 or 3 caluse) and ISC which are all category A/allowed for ASF.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-952258374
Update: I added horizontal scroll and rearranged some labels.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748385017
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I believe it's not allowed.
My interpretation is that you have to state what was the year your contributions were made and it should not be
a) future, because for example it would limit you so that you could not change licencing scheme until it expires
b) `present` verbatim - because it is meaningless in the context of "writer". The `present` only makes sense in the context of "reader" because you do not keep in the file the date when "present" was put. However, the licence file needs the context of "writer", so you need to specify when was the time the licence was last "expected" to be granted by those who have the rights to grant the licence.
We usually perform a bulk update of all licence dates at the beginning of the year. This is also clear expression of intent "yes we want to licence our code under the ASF licence also in 2022". So do not worry about it now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748385017
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I believe it's not allowed.
My interpretation is that you have to state what was the year your contributions were made and it should not be
a) future, because for example it would limit you so that you could not change licencing scheme until it expires
b) `present` verbatim - because it is meaningless in the context of "writer". The `present` only makes sense in the context of "reader" because you do not keep in the file the date when "present" was put. However, the licence file needs the context of "writer", so you need to specify when was the time the licence was last "expected" to be applied by those who have the rights to provide the licence.
We usually perform a bulk update of all licence dates at the beginning of the year. This is also clear expression of intent "yes we want to licence our code under the ASF licence also in 2022". So do not worry about it now.
We usually update all the (C) notices
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r746665054
##########
File path: airflow/www/static/js/tree/InstanceTooltip.jsx
##########
@@ -0,0 +1,145 @@
+/*!
+ * 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.
+ */
+
+/* global moment */
+
+import React from 'react';
+import { Box, Text } from '@chakra-ui/react';
+
+import { formatDateTime } from '../datetime_utils';
+import { getDuration, formatDuration } from './utils';
+
+const InstanceTooltip = ({
+ group,
+ instance: {
+ duration, operator, startDate, endDate, state, taskId, runId,
+ },
+}) => {
+ const isGroup = !!group.children;
+ const groupSummary = [];
+
+ if (isGroup) {
+ const numMap = new Map([
+ ['success', 0],
+ ['failed', 0],
+ ['upstream_failed', 0],
+ ['up_for_retry', 0],
+ ['up_for_reschedule', 0],
+ ['running', 0],
+ ['deferred', 0],
+ ['queued', 0],
+ ['scheduled', 0],
+ ['skipped', 0],
+ ['no_status', 0],
+ ]);
Review comment:
Added 'sensing' for now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r720780406
##########
File path: airflow/www/utils.py
##########
@@ -41,6 +43,47 @@
from airflow.www.widgets import AirflowDateTimePickerWidget
+def datetime_to_string(value: Optional[DateTime]) -> Optional[str]:
Review comment:
I think this can be just (the stdlib) `datetime.datetime` and not restricted to `pendulum`. Also there's a similar function in `views.py` so we should refactor that to use this function as 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pingzh edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
pingzh edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1030375697
Hi @bbovenzi great work, this looks awesome. i am wondering if we can still have the original tree view tab but having this new tree view as a new table view or something else. since with the new table view, it losses the ability to view downstream or upstream of a task. Users are forced to check the Graph Tab at the same time in a new browser tab. For large DAGs, it will be very inconvenient.
Please let me know your thoughts. 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pingzh edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
pingzh edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032984178
thanks @bbovenzi for the demo in https://github.com/apache/airflow/pull/18675#issuecomment-1032806638
can you also try that tree view with several thousand nodes in the DAG?
---
i agree with @potiuk that we do not need the whole graph view in the Tree View. Displaying the upstream/downstream tasks relationship is enough for Tree View and it is succinct.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] KevinYang21 commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
KevinYang21 commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1038732956
Thank you @bbovenzi for the work. I'm all for a more powerful UI and appreciate every bit of excellent effort you guys made/are making.
We already have some great discussion here in this thread and I figure I'll share a bit of my thoughts.
> I do not like this solution because it only shows relations wilth lines which might go up and down arbitrary number of rows (including long lines that will stretch far beyond the screen in big graphs). The big benefit of the solution proposed by @bbovenzi is that you see the direct upstream/downstream task names immediately next to the task in the graph, even if in the grid they will be many, many rows away.
>
> Tracking such relations will be next to impossible if we add lines to the grid.
In the current tree view, we will have a similar problem of having long lines that go beyond the screen, as shown in figure 1. Even in a partial graph view showing three layers of lineage, if the task has a couple hundred of parents/children, we will have similar problem.
IMO viewing full lineage in a huge graph/tree is less useful--people look at the lineage/dependency more when dealing with smaller DAG/subdag/rooted DAG. In those cases, being able to tell the progress and dependencies in one view w/o clutter is very useful and important--that's what brought us the tree view and why it's popular (at least in Airbnb, tree view is visited 13x times than graph view).
Do not get me wrong, I love this table view and definately can envision people using it often, including myself. But with the current implementation (+ optional partial graph view), I'm not convinced that it should be a replacement of the existing tree view, but rather a nice additional table view in our collections to better serve some use cases, e.g. task group heavy DAGs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032917376
> That is why i started searching on how others are doing and found [Erik's library mentioned above](https://observablehq.com/@erikbrinkman/d3-dag-topological). All we need is to add the grid which shows run status to the right side of that visualization which already renders dependencies and kind of vertical list in a single visualization. Eric is also very cooperative in terms of answering any questions.
I do not like this solution because it only show relations wilth lines which might go up and down arbitrary number of rows (including long lines that will stretch far beyond the screen in big graphs). The big benefit of the solution proposed by @bbovenzi is that you see the direct upstream/downstream task names immediately next to the task in the graph, even if in the grid they will be many, many rows away.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032912744
And just to be clear - I think we do not need to bring the whole "graph" view functionality here. The Graph view IMHO should stay where it is, this graph here might really be only a "preview" kind of version of the "full graph" - maybe even a small subset of it showing only direct upstream and downstream tasks - but when you interact with it you could be moved to the full "graph" view.. This way we will not mix the functionalities and have even less clutter but still we will have a nice way for the user to see where the task is in the DAG.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] KevinYang21 edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
KevinYang21 edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1038732956
Thank you @bbovenzi for the work. I'm all for a more powerful UI and appreciate every bit of excellent effort you guys made/are making.
We already have some great discussion here in this thread and I figure I'll share a bit of my thoughts, my apology for the delay.
> I do not like this solution because it only shows relations wilth lines which might go up and down arbitrary number of rows (including long lines that will stretch far beyond the screen in big graphs). The big benefit of the solution proposed by @bbovenzi is that you see the direct upstream/downstream task names immediately next to the task in the graph, even if in the grid they will be many, many rows away.
>
> Tracking such relations will be next to impossible if we add lines to the grid.
In the current tree view, we will have a similar problem of having long lines that go beyond the screen, as shown in figure 1. Even in a partial graph view showing three layers of lineage, if the task has a couple hundred of parents/children, we will have similar problem.
IMO viewing full lineage in a huge graph/tree is less useful--people look at the lineage/dependency more when dealing with smaller DAG/subdag/rooted DAG. In those cases, being able to tell the progress and dependencies in one view w/o clutter is very useful and important--that's what brought us the tree view and why it's popular (at least in Airbnb, tree view is visited 13x times than graph view).
Do not get me wrong, I love this table view and definately can envision people using it often, including myself. But with the current implementation (+ optional partial graph view), I'm not convinced that it should be a replacement of the existing tree view, but rather a nice additional table view in our collections to better serve some use cases, e.g. task group heavy DAGs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r720780836
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
Review comment:
The variable should probably not be named `task_group` 😛
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-937906703
I made some changes from everyone's feedback:
- Updated the hover/group colors to gray instead of blue.
- Extended the vertical date ticks to span the whole dag run duration chart
But two things I'll push back on:
- I think the play icon is still best to indicate a manual run because we use it for the trigger button.
- We already use gray colors to indicate children of Task Groups and on hover. Adding it to even/odd rows would be too noisy.
<img width="677" alt="Screen Shot 2021-10-07 at 10 23 11 AM" src="https://user-images.githubusercontent.com/4600967/136414969-3f69fdd6-2d6c-4ad1-b05d-c72f323ed3c4.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy closed pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #18675:
URL: https://github.com/apache/airflow/pull/18675
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r745491511
##########
File path: airflow/www/static/js/tree/InstanceTooltip.jsx
##########
@@ -0,0 +1,145 @@
+/*!
+ * 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.
+ */
+
+/* global moment */
+
+import React from 'react';
+import { Box, Text } from '@chakra-ui/react';
+
+import { formatDateTime } from '../datetime_utils';
+import { getDuration, formatDuration } from './utils';
+
+const InstanceTooltip = ({
+ group,
+ instance: {
+ duration, operator, startDate, endDate, state, taskId, runId,
+ },
+}) => {
+ const isGroup = !!group.children;
+ const groupSummary = [];
+
+ if (isGroup) {
+ const numMap = new Map([
+ ['success', 0],
+ ['failed', 0],
+ ['upstream_failed', 0],
+ ['up_for_retry', 0],
+ ['up_for_reschedule', 0],
+ ['running', 0],
+ ['deferred', 0],
+ ['queued', 0],
+ ['scheduled', 0],
+ ['skipped', 0],
+ ['no_status', 0],
+ ]);
Review comment:
We need "sensing" in this list I think -- or to decide that 2.3 is where we are going to remove the experimental SmartSensor feature.
##########
File path: airflow/www/static/js/tree/utils.js
##########
@@ -0,0 +1,31 @@
+/*!
+ * 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.
+ */
+
+/* global moment */
+
+// moment will resolve the enddate to now if it is undefined
+export const getDuration = (startDate, endDate) => (
+ moment(endDate || undefined).diff(startDate || undefined)
+);
+
+export const formatDuration = (duration) => {
+ const days = moment.duration(duration).days();
+ // .as('milliseconds') is necessary for .format() to work correctly
+ return `${days > 0 ? `${days}d` : ''}${moment.utc(moment.duration(duration).as('milliseconds')).format('HH:mm:ss')}`;
+};
Review comment:
Rather than a new generic `utils.js` file (which I worry will end up an unstructured dumping ground) these Duration functions would be equally at home in the existing `datetime_utls` I think.
##########
File path: airflow/www/static/js/tree/utils.js
##########
@@ -0,0 +1,31 @@
+/*!
+ * 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.
+ */
+
+/* global moment */
+
+// moment will resolve the enddate to now if it is undefined
+export const getDuration = (startDate, endDate) => (
+ moment(endDate || undefined).diff(startDate || undefined)
+);
+
+export const formatDuration = (duration) => {
+ const days = moment.duration(duration).days();
+ // .as('milliseconds') is necessary for .format() to work correctly
+ return `${days > 0 ? `${days}d` : ''}${moment.utc(moment.duration(duration).as('milliseconds')).format('HH:mm:ss')}`;
Review comment:
No need to call `moment.duration()` twice.
```suggestion
duration = moment.duration(duration)
const days = duration.days();
// .as('milliseconds') is necessary for .format() to work correctly
return `${days > 0 ? `${days}d` : ''}${moment.utc(duration.as('milliseconds')).format('HH:mm:ss')}`;
```
##########
File path: airflow/www/templates/airflow/tree.html
##########
@@ -66,12 +54,7 @@
</div>
</div>
<div class="legend-row">
- <div>
- {% for op in operators %}<span class="legend-item legend-item--no-border">
- <span class="legend-item__swatch legend-item__swatch--circle" style="background: {{ op.ui_color }};"></span>
- {{ op.task_type }}
- </span>{% endfor %}
- </div>
+ <div></div>
Review comment:
What's this empty div needed for in the HTML?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r745734163
##########
File path: airflow/www/package.json
##########
@@ -56,19 +60,29 @@
"webpack-manifest-plugin": "^2.2.0"
},
"dependencies": {
+ "@chakra-ui/react": "^1.6.6",
+ "@emotion/cache": "^11.4.0",
+ "@emotion/react": "^11.4.1",
+ "@emotion/styled": "^11",
"bootstrap-3-typeahead": "^4.0.2",
+ "camelcase-keys": "^7.0.0",
"codemirror": "^5.59.1",
"d3": "^3.4.4",
"d3-shape": "^2.1.0",
"d3-tip": "^0.9.1",
"dagre-d3": "^0.6.4",
"datatables.net": "^1.10.23",
"datatables.net-bs": "^1.10.23",
+ "dayjs": "^1.10.6",
"eonasdan-bootstrap-datetimepicker": "^4.17.47",
+ "framer-motion": "^4",
"jquery": ">=3.5.0",
"jshint": "^2.12.0",
"moment-timezone": "^0.5.28",
"nvd3": "^1.8.6",
+ "react": "^17.0.2",
+ "react-dom": "^17.0.2",
+ "react-icons": "^4.2.0",
Review comment:
(I just picked that somewhat at random -- it may not actually do what we want or be the best option)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748362627
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
Could we interpolate the `2021` to save us the hassle of modifying two months from now?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r720781059
##########
File path: airflow/www/views.py
##########
@@ -219,6 +219,82 @@ def get_date_time_num_runs_dag_runs_form_data(www_request, session, dag):
}
+def task_group_to_tree(task_group, dag, dag_runs, tis):
+ """
+ Create a nested dict representation of this TaskGroup and its children used to construct
+ the Graph.
+ """
+ if isinstance(task_group, BaseOperator):
+ return {
+ 'id': task_group.task_id,
+ 'instances': [wwwutils.encode_ti(ti) for ti in tis if ti.task_id == task_group.task_id],
+ 'label': task_group.label,
+ 'extra_links': dag.get_task(task_group.task_id).extra_links,
Review comment:
Is this needed? `DAG.get_task()` also returns `BaseOperator`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-937906703
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r725699454
##########
File path: airflow/www/package.json
##########
@@ -56,19 +60,29 @@
"webpack-manifest-plugin": "^2.2.0"
},
"dependencies": {
+ "@chakra-ui/react": "^1.6.6",
+ "@emotion/cache": "^11.4.0",
+ "@emotion/react": "^11.4.1",
+ "@emotion/styled": "^11",
"bootstrap-3-typeahead": "^4.0.2",
+ "camelcase-keys": "^7.0.0",
"codemirror": "^5.59.1",
"d3": "^3.4.4",
"d3-shape": "^2.1.0",
"d3-tip": "^0.9.1",
"dagre-d3": "^0.6.4",
"datatables.net": "^1.10.23",
"datatables.net-bs": "^1.10.23",
+ "dayjs": "^1.10.6",
"eonasdan-bootstrap-datetimepicker": "^4.17.47",
+ "framer-motion": "^4",
"jquery": ">=3.5.0",
"jshint": "^2.12.0",
"moment-timezone": "^0.5.28",
"nvd3": "^1.8.6",
+ "react": "^17.0.2",
+ "react-dom": "^17.0.2",
+ "react-icons": "^4.2.0",
Review comment:
Should we add this library or related to [`airflow/licences/`](https://github.com/apache/airflow/tree/main/licenses)?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r725706649
##########
File path: airflow/www/package.json
##########
@@ -56,19 +60,29 @@
"webpack-manifest-plugin": "^2.2.0"
},
"dependencies": {
+ "@chakra-ui/react": "^1.6.6",
+ "@emotion/cache": "^11.4.0",
+ "@emotion/react": "^11.4.1",
+ "@emotion/styled": "^11",
"bootstrap-3-typeahead": "^4.0.2",
+ "camelcase-keys": "^7.0.0",
"codemirror": "^5.59.1",
"d3": "^3.4.4",
"d3-shape": "^2.1.0",
"d3-tip": "^0.9.1",
"dagre-d3": "^0.6.4",
"datatables.net": "^1.10.23",
"datatables.net-bs": "^1.10.23",
+ "dayjs": "^1.10.6",
"eonasdan-bootstrap-datetimepicker": "^4.17.47",
+ "framer-motion": "^4",
"jquery": ">=3.5.0",
"jshint": "^2.12.0",
"moment-timezone": "^0.5.28",
"nvd3": "^1.8.6",
+ "react": "^17.0.2",
+ "react-dom": "^17.0.2",
+ "react-icons": "^4.2.0",
Review comment:
I am not sure about the licenses for the following libraries, but I have not checked all licenses and all libraries.
```
ast-types-flow 0.0.7 released 6 years ago
damerau-levenshtein 1.0.7 released 5 months ago
hoist-non-react-statics 3.3.2 released 2 years ago
type-fest 1.4.0 released 2 months ago
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r727534778
##########
File path: airflow/www/static/js/tree/useTreeData.js
##########
@@ -0,0 +1,79 @@
+/*!
+ * 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.
+ */
+
+/* global treeData, localStorage, fetch, autoRefreshInterval */
+
+import { useState, useEffect, useCallback } from 'react';
+import { useDisclosure } from '@chakra-ui/react';
+import camelcaseKeys from 'camelcase-keys';
+
+import getMetaValue from '../meta_value';
+
+// dagId comes from dag.html
+const dagId = getMetaValue('dag_id');
+const treeDataUrl = getMetaValue('tree_data');
+const numRuns = getMetaValue('num_runs');
+const urlRoot = getMetaValue('root');
+const isPaused = getMetaValue('is_paused');
+
+const areActiveRuns = (runs) => runs.filter((run) => ['queued', 'running', 'scheduled'].includes(run.state)).length > 0;
+
+const useTreeData = () => {
+ const [data, setData] = useState(camelcaseKeys(JSON.parse(treeData), { deep: true }));
+ const defaultIsOpen = isPaused !== 'True' && (JSON.parse(localStorage.getItem('disableAutoRefresh')) || areActiveRuns(data.dagRuns));
+ const { isOpen, onToggle } = useDisclosure({ defaultIsOpen });
+
+ const handleRefresh = useCallback(async () => {
+ const resp = await fetch(`${treeDataUrl}?dag_id=${dagId}&num_runs=${numRuns}&root=${urlRoot}`);
+ let newData = await resp.json();
+ newData = camelcaseKeys(newData, { deep: true });
+ if (JSON.stringify(newData) !== JSON.stringify(data)) {
Review comment:
Python dicts are insertion-ordered (from 3.6 up at least), so this should be mostly stable. The most likely source of issues would be the first refresh, because the initial `treeData` is from the HTML, but subsequent runs use the JSON-based view, and those are more likely to have inconsistencies if we're not careful. I kind of wonder if we should get rid of the initial `treeData` and always render from the JSON view.
The other significant possibility of inconsistencies is SQL natural ordering, but those can be caught incrementally if needed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1039330617
Several thousand nodes (non mapped) in a single DAG is a very very exceptional circumstance -- like 99.9% of DAGs won't even come close to this.
Additionally lets let Brent get this finished and then we can talk about specifics as we (he and I) both made the mistake of showing a work in progress UI change that doesn't capture the full impact idea of the change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1039330617
Several thousand nodes (non mapped) in a single DAG is a very very exceptional circumstance -- like 99.9% of DAGs won't even come close to this.
Additionally lets let Brent get this finished and then we can talk about specifics as we (he and I) both made the mistake of showing a work in progress UI change that doesn't capture the full impact idea of the change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] KevinYang21 edited a comment on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
KevinYang21 edited a comment on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1038732956
Thank you @bbovenzi for the work. I'm all for a more powerful UI and appreciate every bit of excellent effort you guys made/are making.
We already have some great discussion here in this thread and I figure I'll share a bit of my thoughts, my apology for the delay.
> I do not like this solution because it only shows relations wilth lines which might go up and down arbitrary number of rows (including long lines that will stretch far beyond the screen in big graphs). The big benefit of the solution proposed by @bbovenzi is that you see the direct upstream/downstream task names immediately next to the task in the graph, even if in the grid they will be many, many rows away.
>
> Tracking such relations will be next to impossible if we add lines to the grid.
In the current tree view, we will have a similar problem of having long lines that go beyond the screen, as shown in figure 1. Even in a partial graph view showing three layers of lineage, if the task has a couple hundred of parents/children, we will have similar problem.
IMO viewing full lineage in a huge graph/tree is less useful--people look at the lineage/dependency more when dealing with smaller DAG/subdag/rooted DAG. In those cases, being able to tell the progress and dependencies in one view w/o clutter is very useful and important--that's what brought us the tree view and why it's popular (at least in Airbnb, tree view is visited 13x times than graph view).
Do not get me wrong, I love this table view and definately can envision people using it often, including myself. But with the current implementation (+ optional partial graph view), I'm not convinced that it should be a replacement of the existing tree view, but rather a nice additional table view in our collections to better serve some use cases, e.g. task group heavy DAGs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pingzh commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
pingzh commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-1032984178
thanks @bbovenzi for the demo in https://github.com/apache/airflow/pull/18675#issuecomment-1032806638
can you also try that tree view with several thousand nodes in the DAG?
i agree with @potiuk that we do not need the whole graph view in the Tree View. Displaying the upstream/downstream tasks relationship is enough for Tree View and it is succinct.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748384988
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I believe it's not allowed.
My interpretation is that you have to state what was the year your contributions were made and it should not be
a) future, because for example it would limit you so that you could not change licencing scheme until it expires
b) present - because it is meaningless in the context of "writer". The `present` only makes sense in the context of "reader" because you do not keep in the file the date when "present" was put. However, the licence file needs the context of "writer", so you need to specify when was the time the licence was last "expected" to be applied by those who have the rights to provide the licence.
We usually perform a bulk update of all licence dates at the beginning of the year. This is also clear expression of intent "yes we want to licence our code under the ASF licence also in 2022". So do not worry about it now.
We usually update all the (C) notices
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I believe it's not allowed.
My interpretation is that you have to state what was the year your contributions were made and it should not be
a) future, because for example it would limit you so that you could not change licencing scheme until it expires
b) present - because it is meaningless in the context of "writer". The `present` only makes sense in the context of "reader" because you do not keep in the file the date when "present" was put. However, the licence file needs the context of "writer", so you need to specify when was the time the licence was last "expected" to be applied by those who have the rights to provide the licence.
We usually perform a bulk update of all licence dates at the beginning of the year. This is also clear expression of intent "yes we want to licence our code under the ASF licence also in 2022". So do not worry about it now.
We usually update all the (C) notices
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#discussion_r748385017
##########
File path: airflow/www/webpack.config.js
##########
@@ -35,6 +36,22 @@ const JS_DIR = path.resolve(__dirname, './static/js');
// noinspection JSUnresolvedVariable
const BUILD_DIR = path.resolve(__dirname, './static/dist');
+// Convert licenses json into a standard format for LICENSES.txt
+const formatLicenses = (packages) => {
+ let text = `Apache Airflow
+Copyright 2016-2021 The Apache Software Foundation
Review comment:
I believe it's not allowed.
My interpretation is that you have to state what was the year your contributions were made and it should not be
a) future, because for example it would limit you so that you could not change licencing scheme until it expires
b) `present` verbatim - because it is meaningless in the context of "writer". The `present` only makes sense in the context of "reader" because you do not keep in the file the date when "present" was put. However, the licence file needs the context of "writer", so you need to specify when was the time the licence was last "expected" to be granted by those who have the rights to grant the licence.
We usually perform a bulk update of all licence dates at the beginning of the year. This is also clear expression of intent "yes we want to licence our code under the ASF licence also in 2022". So do not worry about it now.
We usually update all the (C) notices
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #18675: New Tree View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #18675:
URL: https://github.com/apache/airflow/pull/18675#issuecomment-963741391
Now with testing for our react components. Setting up jest in `airflow/www` will also allow us to write tests for our current js files too.
<img width="661" alt="Screen Shot 2021-11-08 at 7 54 50 PM" src="https://user-images.githubusercontent.com/4600967/140847344-d0f24e63-53b1-436b-b6dd-6d9e4cfe8b47.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org