Skip to content

Only return non-None task summaries for grid view #23924

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.

Sign up for GitHub

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

Closed
jedcunningham wants to merge 3 commits into from
Closed

Only return non-None task summaries for grid view #23924

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
688117b
Only return non-None task summaries for grid view
jedcunningham May 25, 2022
abfb033
Fix static checks
jedcunningham May 26, 2022
bc056ff
Update airflow/www/views.py
jedcunningham May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion airflow/www/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,11 @@ def task_group_to_grid(task_item_or_group, dag, dag_runs, session):
if isinstance(task_item_or_group, AbstractOperator):
return {
'id': task_item_or_group.task_id,
'instances': [wwwutils.get_task_summary(dr, task_item_or_group, session) for dr in dag_runs],
'instances': [
ts
for ts in (wwwutils.get_task_summary(dr, task_item_or_group, session) for dr in dag_runs)
if ts is not None
],
Comment on lines +261 to +265
Copy link
Contributor

@dstandish dstandish May 26, 2022

Choose a reason for hiding this comment

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

just to throw out another option,

Suggested change
'instances': [
ts
for ts in (wwwutils.get_task_summary(dr, task_item_or_group, session) for dr in dag_runs)
if ts is not None
],
'instances': filter(
lambda x: x is not None,
(wwwutils.get_task_summary(dr, task_item_or_group, session) for dr in dag_runs),
),

don't know if it also needs to be wrapped with list. i know we tend to prefer comprehension over map etc, but in this case it saves you that ts variable, which, since we're already iterating over DRs, is a tiny bit more cognitive load i thought.

Copy link
Member

@uranusjr uranusjr May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW if we don’t need to care about empty summary (or if we’re OK to also filter those out), we can simply do

filter(None, (...))

But yes I believe an additional list call is necessary here, so comprehension is better IMO.

Copy link
Contributor

@dstandish dstandish May 26, 2022

Choose a reason for hiding this comment

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

nice, i was thinking there must be a shorthand of some kind for this

Copy link
Member

@potiuk potiuk May 26, 2022

Choose a reason for hiding this comment

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

Alternative fix here: #23932 - I think we shoudl slowly start add typing also to views.py as this precisely the kind of errors that would have been prevented if we had typing here in the first place.

Copy link
Contributor

@bbovenzi bbovenzi May 26, 2022

Choose a reason for hiding this comment

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

More type checking is always nice. I've been meaning to add typescript to the UI.

Copy link
Member

@potiuk potiuk May 26, 2022

Choose a reason for hiding this comment

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

More type checking is always nice. I've been meaning to add typescript to the UI.

Oh yeah. Absolutely. Typescript is soooo much better than plain javascript. One thing that it makes easier - is for anyone to contribute more fixes in an easy way. (once we have a built-in automation to transpile it, which is super-easy with yarn etc). I think lack of typing support, autocomplete and verification makes it really difficult for "newcomers" to reason about their changes and it's so much easier to contribute small fixes there.

You somehow loose that when you get familiar, when you put yourselve in the shoes of new contributor - it's a world of difference.

'label': task_item_or_group.label,
'extra_links': task_item_or_group.extra_links,
'is_mapped': task_item_or_group.is_mapped,
Expand Down