Skip to content

Conversation

@bbovenzi
Copy link
Contributor

@bbovenzi bbovenzi commented Mar 9, 2022

Add a details drawer to the right side of the grid view to quickly see details and actions for the DAG / Dag Run / Task Instance. This replaces the modal when clicking on a dag run or task instance. (Modals still exist for graph and gantt views)

Benefits:

  • Simplify tooltips to only show basic at-a-glance info, and rely on the drawer for more advanced details
  • Show details in context of what run or task you are looking at
  • Dag Run and Task Instance actions in context of what Task/Run you are changing
  • Run and Task actions is a seamless experience without multiple page redirects. And one can immediately see changes take place.

Next Up:

  • The side drawer for a mapped task will include a table of all of its mapped instances with their respective actions
  • A better DAG details side drawer. In this PR, it is a table of some basic details, but I would like for it to be an overview of your DAG and the runs/tasks that are currently being displayed in the grid view

Closes #19938
Closes #21428
Related #20382
Closes #19674

Other notes:
Python isn't my strong suit so any help with the changes I had to make to the webserver (views.py and utils.py) would be greatly appreciated.

Dag Details:
Screen Shot 2022-03-17 at 12 40 31 PM

Run Details:
Screen Shot 2022-03-17 at 12 37 11 PM

Task Instance Details:
Screen Shot 2022-03-17 at 12 37 02 PM

Navigating between details:
Mar-17-2022 12-56-37

Scrolling:
Mar-17-2022 12-46-07

Marking a Task Failed:
Mar-17-2022 12-51-06

Clearing a DAG Run:
Mar-17-2022 12-51-57

Changing Timezone:
Mar-17-2022 21-03-22


^ 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 Mar 9, 2022
Duration:
{' '}
{endDate && formatDateTime(endDate)}
{formatDuration(duration || getDuration(startDate, endDate))}
Copy link
Member

Choose a reason for hiding this comment

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

What does this show when the task is still in progress?

Do we not show end date anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

endDate is in the details but not in the tooltip.
When in progress it will use Date.now() instead of enddate

@bbovenzi bbovenzi force-pushed the mapped-task-drawer branch from 74d739c to 1a9e6f3 Compare March 15, 2022 00:50
@bbovenzi bbovenzi marked this pull request as ready for review March 17, 2022 17:10
@bbovenzi bbovenzi requested a review from ryanahamilton as a code owner March 17, 2022 17:10
}
}

export function callModalDag(dag) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only called this in the grid view, so we can fully replace it

</div>
</div>
</div>
<!-- Modal for DAG run -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now all in React instead.

@bbovenzi bbovenzi added the AIP-38 Modern Web Application label Mar 17, 2022
@bbovenzi bbovenzi added this to the Airflow 2.3.0 milestone Mar 17, 2022
@eladkal
Copy link
Contributor

eladkal commented Mar 17, 2022

Looks really good!

2 comments:

  1. for me the arrow is associated with the browser arrows (back / forward). In this case the action is close so wouldn't X icon be more suitable?
    Screen Shot 2022-03-17 at 22 07 13

  2. I think in the confirm pop-ups we should default the confirm button

@ashb
Copy link
Member

ashb commented Mar 17, 2022

  1. I think in the confirm pop-ups we should default the confirm button

What did the current confirm page default to?

@bbovenzi
Copy link
Contributor Author

  1. I think in the confirm pop-ups we should default the confirm button

What did the current confirm page default to?

It highlighted "Cancel" by default.

@eladkal
Copy link
Contributor

eladkal commented Mar 20, 2022

Played with it a bit in local env this is really really good!

Have a few suggestions/bugs all are minor:

  1. [Suggestion] Can we have the ability to easily copy to clipboard?
    copy2
    To give example of what i had in mind:
    copy

  2. [Tweak] Need to fix the name to Grid also in:
    Screen Shot 2022-03-20 at 21 59 25

  3. [Bug] When changing Timezone there is a weird "jump" of values :
    2022-03-20_22-01-25
    Also the timezone changes only in start date but not in Timezone (last row in the table) - why?
    Is this the timezone the DAG is defined with in the code? If so maybe better to call it DAG defined timezone or something like that?

  4. [Bug] When opening the panel of a fresh task (state None) the panel isn't refreshed while the task progresses. So when the task finishes the started/ended values are empty (you must refresh the screen manually to solve this)
    The values in the panel should be refreshed when with the auto-refresh property.
    (Edit: noticed also the status left as no-status though that is Success and probably duration is also not updated.. so I guess the whole panel isn't being updated with the auto refresh functionality)
    Screen Shot 2022-03-20 at 22 05 58

  5. [Suggestion] I got a bit lost when clicked on Filter upstream. It was impossible to return where I was before I had to enter the Grid view and repeat my steps. Is there a way to make a better experience with it?

  6. [Suggestion] There are two "More details" one that leads to DAG details and one that leads to Task Details. Can we simply rename it to be "DAG details"/"Task Details"?
    The same goes for "Show Details Panel" It shows different panel for Task/DAG but the button has the same name.

  7. [Bug] I'm not really sure how I managed to do that but I get a tooltip of Show when the button says hide and vise versia. The tool tip is redundant any anyway isn't it?
    Screen Shot 2022-03-20 at 22 17 34

@bbovenzi bbovenzi force-pushed the mapped-task-drawer branch from 74fada2 to 60445c4 Compare March 21, 2022 17:19
@bbovenzi
Copy link
Contributor Author

bbovenzi commented Mar 21, 2022

Thanks for all the testing @eladkal!

  1. I can do that in a later PR
  2. Already fixed in a different PR
  3. The whole DAG details page will change pretty soon, so I won't fix for now
  4. I think this should be a later PR. It should totally happen, but to do correctly I think we need to redo how tree_data works in views.py. I will absolutely do it before 2.3.0, but I think making that change to this PR will make it even harder than it already is to review all of the code changes. (Edit: I figured out a workable solution for this PR.)
  5. Added a "Reset Root" button if you filter upstream
  6. Updated all the "More Details" to specifiy which details we are linking to
  7. That title was backwards and now redundant. Removed.

@eladkal
Copy link
Contributor

eladkal commented Mar 21, 2022

Great. Since I'm not familiar with the UI side of the code I can't do a proper code review but I can confirm that functionality wise it work as expected.

@bbovenzi bbovenzi requested a review from uranusjr March 23, 2022 16:57
@uranusjr
Copy link
Member

Python logic looks good to me, but we can probably do better sharing logic between HTML and JSON requests.

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.

For the Python part

@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 Mar 25, 2022
@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 main or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr
Copy link
Member

I added a commit to fix the KeyError. There are still a couple of failures relating to argument count that need fixing, I’ll leave those to you.

bbovenzi and others added 24 commits March 31, 2022 16:57
- Remove details panel title
- Add button to reset root
- Make "More Details" buttons more specific
- Specify timezone as DAG timezone
None of these changes were relevant to this PR. Better to be done separately.
- useState vs useDisclosure
- Remove extraneous elements
- Copy changes
- Wire up params for runTask
- Breadcrumb padding
- Pass 'Accept' headers for json returns
- Consolidate more endpoints to return json or redirect
- add readme
-  rename context providers to avoid confusion with Airflow Providers
@bbovenzi bbovenzi force-pushed the mapped-task-drawer branch from bc06078 to ea3551d Compare March 31, 2022 20:57
@bbovenzi bbovenzi merged commit 2bb26a3 into main Mar 31, 2022
@bbovenzi bbovenzi deleted the mapped-task-drawer branch March 31, 2022 21:53
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-38 Modern Web Application area:dynamic-task-mapping AIP-42 area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

9 participants