Only return non-None task summaries for grid view #23924
Conversation
If we do return None's, it breaks the grid view when a new task is added to a DAG.
|
Would it be more straightforward to do this at the front end instead? |
|
Maybe, but I'm not sure sending a bunch of nulls out to the front end is really useful. This restores the behavior pre #23813, as I'm pretty sure this new behavior wasn't intentional. |
Co-authored-by: Tzu-ping Chung <[email protected]>
jedcunningham
requested review from
ashb,
bbovenzi and
ryanahamilton
as code owners
| '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 | ||
| ], |
There was a problem hiding this comment.
just to throw out another option,
| '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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
nice, i was thinking there must be a shorthand of some kind for this
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
More type checking is always nice. I've been meaning to add typescript to the UI.
There was a problem hiding this comment.
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.
|
I'll try out this and Jarek's solution in a few hours. I'll also open a PR to make sure the UI handles this issue better. We should definitely show a blank task instance in the grid if it doesn't exist in a dag run but does in others. |
Cool. |
|
UI fix: #23939 |
|
Closing this in favor of #23947, which fixes the same bug but also significantly improves response time. |
6 participants
If we do return None's, it breaks the grid view when a new task is added
to a DAG.
Fixes: #23908