Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto refresh on Tree View #15474

Merged
merged 14 commits into from
Apr 23, 2021
Merged

Conversation

bbovenzi
Copy link
Contributor

@bbovenzi bbovenzi commented Apr 21, 2021

Auto-refresh on Graph View was a very popular feature when 2.0.0 was released. This PR adds auto-refresh for Tree View.

  • add a new tree_data endpoint to ping for updates every 3 seconds
  • only rerender if the task instances have changed
  • turn off autorefresh after there are no more running dag runs

pr-gif


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Apr 21, 2021
@bbovenzi bbovenzi marked this pull request as draft April 21, 2021 17:29
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably too much duplication between this new method and the existing tree view that we should pull out in to a function and call from both places.

airflow/www/templates/airflow/tree.html Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
bbovenzi and others added 5 commits April 22, 2021 11:24
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
@bbovenzi bbovenzi marked this pull request as ready for review April 22, 2021 15:33
@@ -459,6 +459,97 @@ def add_user_permissions_to_dag(sender, template, context, **extra): # noqa pyl
context['dag'] = dag


def get_tree_data(dag_runs, dag, base_date):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def get_tree_data(dag_runs, dag, base_date):
def get_tree_data(dag_runs: Iterable[DagRun], dag: DAG, base_date: DateTime):

(I think those types should be in scope and correct)

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

airflow/www/templates/airflow/graph.html Show resolved Hide resolved
airflow/www/static/js/tree.js Show resolved Hide resolved
airflow/www/static/css/tree.css Outdated Show resolved Hide resolved
@ryanahamilton
Copy link
Contributor

Found a little visual jank on wide screens. Looks like a float that wraps when the SVG body doesn't utilize the full screen width.

Screen Recording 2021-04-22 at 04 26 22 PM

@bbovenzi
Copy link
Contributor Author

Good catch. I added a min-width to the refresh actions so there is always space for the loading dots.

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some coding style suggestions that might help get rid of pylint’s too-many-locals warning as well (no idea how many is too many). Nothing logical incorrect from what I can tell.

Comment on lines 2047 to 2049
dates = sorted(dag_runs.keys())
max_date = max(dates) if dates else None
min_date = min(dates) if dates else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dates = sorted(dag_runs.keys())
max_date = max(dates) if dates else None
min_date = min(dates) if dates else None
max_date = max(dag_runs, default=None)
min_date = min(dag_runs, default=None)

Comment on lines 2051 to 2054
tis = dag.get_task_instances(start_date=min_date, end_date=base_date)
task_instances: Dict[Tuple[str, datetime], models.TaskInstance] = {}
for ti in tis:
task_instances[(ti.task_id, ti.execution_date)] = ti
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tis = dag.get_task_instances(start_date=min_date, end_date=base_date)
task_instances: Dict[Tuple[str, datetime], models.TaskInstance] = {}
for ti in tis:
task_instances[(ti.task_id, ti.execution_date)] = ti
task_instances = {
(ti.task_id, ti.execution_date): ti
for ti in dag.get_task_instances(start_date=min_date, end_date=base_date)
}

Comment on lines 2032 to 2035
if base_date:
base_date = timezone.parse(base_date)
else:
base_date = dag.get_latest_execution_date() or timezone.utcnow()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if base_date:
base_date = timezone.parse(base_date)
else:
base_date = dag.get_latest_execution_date() or timezone.utcnow()
try:
base_date = timezone.parse(request.args["base_date"])
except (KeyError, ValueError):
base_date = dag.get_latest_execution_date() or timezone.utcnow()

and remove the base_date line above. There’s a similar one in the other view as well.

Copy link
Contributor

@ryanahamilton ryanahamilton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bunch of local testing on this—it works great and I think people are really going to love it!

@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 23, 2021
airflow/www/views.py Outdated Show resolved Hide resolved
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
@ryanahamilton ryanahamilton merged commit 76105c1 into apache:master Apr 23, 2021
@ryanahamilton ryanahamilton deleted the refresh-tree-view branch April 23, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants