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 2020/10/19 08:38:57 UTC
[GitHub] [airflow] scrdest opened a new pull request #11652: Support for sorting DAGs in the web UI
scrdest opened a new pull request #11652:
URL: https://github.com/apache/airflow/pull/11652
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Adds support for sorting DAGs in the UI by the ID, Schedule (next run, really) and Owner, ASC/DESC.
The sorting is done on the DB level and parameterized by two new URL parameters - sortBy for the model attribute & orderBy for the type of ordering (ascending/descending).
The default behavior is to sort by Dag ID and order by ascending, and therefore should not come as a surprise to current users.
The column headings in the DAG view have been turned to hyperlinks that pass the parameters for the sorting. For more predictable UX, when switching before sorting attributes (or when none is passed), clicking on a header will always sort it by ASC first. Clicking on the same header again after the page reloads will toggle it to DESC instead. Visually:
```
(None)->[Dag ID] => (Dag ID ASC)
(Dag ID ASC) -> [Dag ID] => (Dag ID DESC)
(Dag ID DESC) -> [Dag ID] => (Dag ID ASC)
(Dag ID ASC) -> [Owner] => (Owner ASC)
(Dag ID DESC) -> [Owner] => (Owner ASC)
```
Unfortunately, sorting by the most recently executed run as requested by #8459 is *not* supported yet. The current options were fairly straightforward to handle since the logic that populates the DAG list currently fetches a set of DagModels - this is just an exercise in filtering. Fetching and sorting DagRuns ran the risk of adding a fair bit of extra processing overhead and complicating the logic.
closes: #8459
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r512182055
##########
File path: airflow/www/utils.py
##########
@@ -302,6 +311,48 @@ def dag_run_link(attr):
'<a href="{url}">{run_id}</a>').format(url=url, run_id=run_id) # noqa
+def dag_query_for_key(sorting_key):
+ """Maps sorting key param in the URL params to DB queries on appropriate attributes."""
+ dag_query_key_map = {
+ 'dag_id': DagModel.dag_id,
+ 'owner': DagModel.owners,
+ 'schedule': DagModel.next_dagrun,
+ # <- add any extra (URL param)->(DagModel attr) mappings here
+ }
Review comment:
I see your point, but I feel like the suggested way is a bigger PITA from a long-term maintenance perspective - you have to add each new sortable in duplicate. It also seems less intuitive when looking at the URL - the current setup basically just replicates the underlying SQL. All in all, I was trying to make the two bits as independently testable as possible.
I may be getting ahead of myself, but it should also make things easier if someone wanted to sort by multiple columns - e.g. ASC by ID and DESC by owner would be `orderBy=asc;desc` and then you could just `zip()` it with `sortBy`'s items.
Re: the module location, point taken. I will move things around as appropriate. I was kind of hoping someone would slap me and point me to the right place if this one wasn't it, so - appreciated!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jkmalek commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
jkmalek commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r607769776
##########
File path: airflow/www/views.py
##########
@@ -332,6 +332,47 @@ def get_downstream(task):
]
+def dag_query_for_key(sorting_key):
+ """Maps sorting key param in the URL params to DB queries on appropriate attributes."""
+ dag_query_key_map = {
+ 'dag_id': DagModel.dag_id,
+ 'owner': DagModel.owners,
+ 'schedule': DagModel.next_dagrun,
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r508079706
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -76,9 +76,21 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
Review comment:
It would probably be good to add icons to these links to give them a bit more visual distinction.
```suggestion
DAG<span class="material-icons">unfold_more</span>
```
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -76,9 +76,21 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ </a>
+ </th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='schedule', orderBy='asc' if request.args.get('sortBy') != 'owner' or request.args.get('orderBy', 'schedule') != 'asc' else 'desc' ) }}">
Review comment:
Appear to be some typos here:
```suggestion
<a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='schedule', orderBy='asc' if request.args.get('sortBy') != 'schedule' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
```
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -76,9 +76,21 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ </a>
+ </th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='schedule', orderBy='asc' if request.args.get('sortBy') != 'owner' or request.args.get('orderBy', 'schedule') != 'asc' else 'desc' ) }}">
+ Schedule
Review comment:
```suggestion
Schedule<span class="material-icons">unfold_more</span>
```
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -76,9 +76,21 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ </a>
+ </th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='schedule', orderBy='asc' if request.args.get('sortBy') != 'owner' or request.args.get('orderBy', 'schedule') != 'asc' else 'desc' ) }}">
+ Schedule
+ </a>
+ </th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='owner', orderBy='asc' if request.args.get('sortBy') != 'owner' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ Owner
Review comment:
```suggestion
Owner<span class="material-icons">unfold_more</span>
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r535266580
##########
File path: airflow/www/views.py
##########
@@ -449,6 +490,8 @@ def get_int_arg(value, default=0):
arg_search_query = request.args.get('search')
arg_tags_filter = request.args.getlist('tags')
arg_status_filter = request.args.get('status')
+ arg_sorting_key = request.args.get('sortBy')
+ arg_orderby_key = request.args.get('orderBy')
Review comment:
These two params are two confusing/similar, and clash a bit with standard SQL meanings, since the sortBy value is the term after the `ORDER BY`.
Could we instead have `orderBy` and `orderDir` (direction).
(I also would have personally done it as `orderBy=dag_id` for asc and `orderBy=-dag_id` for desc. It means you can then order by multiple columns with `orderBy=col_a,-col_b` or `orderBy=col_a&orderBy=-col_b`. But feel free to leave it as two params, it's fine as it is)
##########
File path: airflow/www/views.py
##########
@@ -332,6 +332,47 @@ def get_downstream(task):
]
+def dag_query_for_key(sorting_key):
+ """Maps sorting key param in the URL params to DB queries on appropriate attributes."""
+ dag_query_key_map = {
+ 'dag_id': DagModel.dag_id,
+ 'owner': DagModel.owners,
+ 'schedule': DagModel.next_dagrun,
Review comment:
I think I'd rather these values matched the column names -- we're already having to provide the "Readable String" and the order by in the template.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r511151414
##########
File path: airflow/www/utils.py
##########
@@ -302,6 +311,48 @@ def dag_run_link(attr):
'<a href="{url}">{run_id}</a>').format(url=url, run_id=run_id) # noqa
+def dag_query_for_key(sorting_key):
+ """Maps sorting key param in the URL params to DB queries on appropriate attributes."""
+ dag_query_key_map = {
+ 'dag_id': DagModel.dag_id,
+ 'owner': DagModel.owners,
+ 'schedule': DagModel.next_dagrun,
+ # <- add any extra (URL param)->(DagModel attr) mappings here
+ }
Review comment:
(If you think the current way is clearer, tell me. But I do think the location is wrong for this, as it is specific to one particular view.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-891193935
@scrdest, I'd like to get this in for 2.2.0. Could you take a look at fixing the merge conflicts? Just a heads up, `tests/www/test_views.py` has been split apart so that piece will need some closer attention.
As for the duplication with the stuff in the REST API, they aren't too different already so I think we can handle it later đź‘Ť.
--
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 #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r590419362
##########
File path: tests/www/test_views.py
##########
@@ -62,7 +62,14 @@
from airflow.utils.timezone import datetime
from airflow.utils.types import DagRunType
from airflow.www import app as application
-from airflow.www.views import ConnectionModelView, get_safe_url, truncate_task_duration
+from airflow.www.views import (
+ ConnectionModelView,
+ build_dag_sorting_query,
+ dag_query_for_key,
+ get_safe_url,
+ query_ordering_transform_for_key,
+ truncate_task_duration
Review comment:
This will fix the failing static check (Black).
```suggestion
truncate_task_duration,
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-722234432
@kaxil - rebased.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RosterIn commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
RosterIn commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r584926735
##########
File path: tests/www/test_views.py
##########
@@ -62,7 +62,14 @@
from airflow.utils.timezone import datetime
from airflow.utils.types import DagRunType
from airflow.www import app as application
-from airflow.www.views import ConnectionModelView, get_safe_url, truncate_task_duration
+from airflow.www.views import (
+ ConnectionModelView,
+ build_dag_sorting_query,
+ dag_query_for_key,
+ query_ordering_transform_for_key,
+ get_safe_url,
+ truncate_task_duration
Review comment:
```suggestion
get_safe_url,
query_ordering_transform_for_key,
truncate_task_duration
```
to fix the failing test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-713791940
Nice, I like the changing icons based on the sort state!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r534635052
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,39 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering == 'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') != attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}"
+ class="js-tooltip"
+ role="tooltip"
+ title="Press to sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}."
Review comment:
One last nit… Can we make this tooltip less verbose? The "Press to" can be inferred by the interaction (hover) that invokes it.
```suggestion
title="Sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}."
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r584975190
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,39 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering == 'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') != attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}"
+ class="js-tooltip"
+ role="tooltip"
+ title="Press to sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}."
Review comment:
Appears that this suggestion didn't get applied.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-859107901
>
>
> @scrdest can you fix these merge conflicts? If so we'll target 2.2 for this release.
>
> @ashb bumping since you requested changes
I'm happy to in principle, but @uranusjr 's comment made me leave this on the back-burner because I thought it would wind up getting closed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-859505109
My comment was not meant to dismiss the PR, but intended to provide some context so we could investigate whether it would be possible to move the filtering logic from Jinja2 to the view functions (in Python). Sorry if it sent the wrong signal.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jkmalek commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
jkmalek commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r607736651
##########
File path: airflow/www/views.py
##########
@@ -449,6 +490,8 @@ def get_int_arg(value, default=0):
arg_search_query = request.args.get('search')
arg_tags_filter = request.args.getlist('tags')
arg_status_filter = request.args.get('status')
+ arg_sorting_key = request.args.get('sortBy')
+ arg_orderby_key = request.args.get('orderBy')
Review comment:
Went with (B) for time being since it requires less changes and I'm already worried about rebases messing up other changes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-787558832
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r518387007
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,51 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering == 'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') != attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}">
+ {{ name }}
+ <span id="sorting-tip-{{ name }}" class="tooltip" role="tooltip" >
Review comment:
You can simplify this and circumvent adding this tooltip span by adding a `class="js-tooltip"` and a `title="{{ tooltip }}"` attributes to the parent anchor.
IMO, the current tooltip text is a bit too verbose. I would suggest dropping the "Column not currently sorted." and "Column sorted by…" all together and just provide description of what clicking on will do: "Sort by ascending owner" etc.
```jinja
<a
…
class="js-tooltip"
title="Sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}"
>
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jkmalek commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
jkmalek commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-892689960
@jedcunningham rebased and fixed up where necessary; thanks for the heads-up on view tests!
--
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] jedcunningham commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r691614791
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,39 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering == 'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderDir', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('orderDir') != attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('orderDir') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
Review comment:
```suggestion
{% set curr_ordering = ('unsorted' if request.args.get('orderBy') != attr_name else request_ordering ) %}
{% set new_ordering = ('asc' if request.args.get('orderBy') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
```
##########
File path: tests/www/views/test_views_dags.py
##########
@@ -0,0 +1,77 @@
+#
+# 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.
+
+
+from airflow.models.dag import DagModel
+from airflow.www.views import build_dag_sorting_query, dag_query_for_key, query_ordering_transform_for_key
+
+
+def test_dag_sorting_query():
+
+ dag_id_query = dag_query_for_key(sorting_key='dag_id')
+ dag_id_query_casetest = dag_query_for_key(sorting_key='dAg_Id')
+
+ assert dag_id_query == DagModel.dag_id
+ assert dag_id_query_casetest == dag_id_query
+
+ owner_query = dag_query_for_key(sorting_key='owner')
+ owner_query_casetest = dag_query_for_key(sorting_key='oWnEr')
+
+ assert owner_query == DagModel.owners
+ assert owner_query != dag_id_query
+ assert owner_query_casetest == owner_query
+
+ schedule_query = dag_query_for_key(sorting_key='next_dagrun')
+
+ assert schedule_query == DagModel.next_dagrun
+ assert schedule_query != dag_id_query
+ assert schedule_query != owner_query
Review comment:
nit: This whole section could utilize `pytest.mark.parameterize` which would make it cleaner imo, e.g. (untested):
```suggestion
@pytest.mark.parameterize(
"key, expected_query",
[
["dag_id", DagModel.dag_id],
["dAg_Id", DagModel.dag_id],
["owner", DagModel.owners],
["oWnEr", DagModel.owners],
["next_dagrun", DagModel.next_dagrun],
["NEXT_DAGRUN", DagModel.next_dagrun],
]
)
def test_dag_query_for_key(key, expected_query):
assert dag_query_for_key(sorting_key=key) == expected_query
```
The same pattern could be used for `query_ordering_transform_for_key` and `build_dag_sorting_query`.
--
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 #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-919075241
@scrdest Can you address the requested changes please
--
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 pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-814957276
I like this, but a thing that worries me is we’ve recently merged filtering logic for the API, and with this we’ll have two mostly identical sets of logic for sorting DAGs, one in Python (for the API), and one in Jinja2 (for the web UI). Would it be possible to move most of the logic into the views instead? This would make it much easier to potentially refactor the two to use one implementation (see #15072).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryw commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryw commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-717490162
thanks for working on this @scrdest and your first Airflow contribution :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r513677296
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -78,9 +78,48 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ <span class="material-icons">
+ {% if request.args.get('orderBy', 'none') == 'desc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_less
+ {% elif request.args.get('orderBy', 'none') == 'asc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_more
+ {% else %}
+ unfold_more
+ {% endif %}
+ </span>
+ </a>
Review comment:
Added.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r511152027
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -78,9 +78,48 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ <span class="material-icons">
+ {% if request.args.get('orderBy', 'none') == 'desc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_less
+ {% elif request.args.get('orderBy', 'none') == 'asc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_more
+ {% else %}
+ unfold_more
+ {% endif %}
+ </span>
+ </a>
Review comment:
Ah, yes. Given the text-to-icon strategy employed, we should add `aria-hidden="true"` to the `<span />`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r534857054
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,39 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering == 'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') != attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}"
+ class="js-tooltip"
+ role="tooltip"
+ title="Press to sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}."
Review comment:
Done!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r512183538
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -78,9 +78,48 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ <span class="material-icons">
+ {% if request.args.get('orderBy', 'none') == 'desc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_less
+ {% elif request.args.get('orderBy', 'none') == 'asc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_more
+ {% else %}
+ unfold_more
+ {% endif %}
+ </span>
+ </a>
Review comment:
I've been toying with the idea of slapping a macro onnit, but I wanted to KISS for now. More than happy to refactor it to use the macro. Will add the arias 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-712173316
/cc @ryanahamilton
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r511152368
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -78,9 +78,48 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ <span class="material-icons">
+ {% if request.args.get('orderBy', 'none') == 'desc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_less
+ {% elif request.args.get('orderBy', 'none') == 'asc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_more
+ {% else %}
+ unfold_more
+ {% endif %}
+ </span>
+ </a>
Review comment:
Perhaps also a tooltip to say "Sort by $col desc" 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-739901500
Sorry, meant to mark this as 2.1 since it's an entirely new feature.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r511151210
##########
File path: airflow/www/utils.py
##########
@@ -302,6 +311,48 @@ def dag_run_link(attr):
'<a href="{url}">{run_id}</a>').format(url=url, run_id=run_id) # noqa
+def dag_query_for_key(sorting_key):
+ """Maps sorting key param in the URL params to DB queries on appropriate attributes."""
+ dag_query_key_map = {
+ 'dag_id': DagModel.dag_id,
+ 'owner': DagModel.owners,
+ 'schedule': DagModel.next_dagrun,
+ # <- add any extra (URL param)->(DagModel attr) mappings here
+ }
Review comment:
It might be easier/less code to have a single `sort_by` and have this as
```suggestion
dag_query_key_map = {
'dag_id': DagModel.dag_id,
'-dag_id': DagModel.dag_id.desc(),
'owner': DagModel.owners,
'-owner': DagModel.owners.desc(),
'schedule': DagModel.next_dagrun,
'-schedule': DagModel.next_dagrun.desc(),
# <- add any extra (URL param)->(DagModel attr) mappings here
}
```
That way we don't need to have extra sorting_order param. I think this makes the code and the URI simpler.
Additionally this order probably doesn't belong in this file -- but instead in the views.py file, probably in the `home` function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-858799643
@scrdest can you fix these merge conflicts? If so we'll target 2.2 for this release.
@ashb bumping since you requested changes
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-891320564
@jedcunningham alright, will do; I was going to action this as of @uranusjr 's comment but, well, Life happened.
--
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] scrdest commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-814070795
@ashb addressed the naming issues you've raised; anything else outstanding?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-729022559
[The Workflow run](https://github.com/apache/airflow/actions/runs/368445316) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r509259332
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -76,9 +76,21 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ </a>
+ </th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='schedule', orderBy='asc' if request.args.get('sortBy') != 'owner' or request.args.get('orderBy', 'schedule') != 'asc' else 'desc' ) }}">
Review comment:
Good catch, thanks! Fixed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-739859575
Agreed, not critical as it's a new feature.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r511149526
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -78,9 +78,48 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
+ <span class="material-icons">
+ {% if request.args.get('orderBy', 'none') == 'desc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_less
+ {% elif request.args.get('orderBy', 'none') == 'asc' and request.args.get('sortBy') == 'dag_id' %}
+ expand_more
+ {% else %}
+ unfold_more
+ {% endif %}
+ </span>
+ </a>
Review comment:
This is quite the mouthfull, and repeated three times. Luckily there is `macro` to the rescue.
```jinja
{% macro sorted_th_link(name, attr_name) -%}
<a href="{{ url_for('Airflow.index',
status=request.args.get('status', 'all'),
search=request.args.get('search', None),
tags=request.args.get('tags', None),
sortBy=attr_name, orderBy='asc' if request.args.get('sortBy') != attr_name or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
{{ name }}
<span class="material-icons">
{% if request.args.get('orderBy', 'none') == 'desc' and request.args.get('sortBy') == attr_name %}
expand_less
{% elif request.args.get('orderBy', 'none') == 'asc' and request.args.get('sortBy') == attr_name %}
expand_more
{% else %}
unfold_more
{% endif %}
</span>
</a>
{%- endmacro %}
```
And then we use it like this:
```suggestion
{{ sorted_th_link("DAG", "dag_id") }}
```
Similarly for the other two headers.
@ryanahamilton Should we set aria tags on the span/link?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jkmalek commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
jkmalek commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-892689960
@jedcunningham rebased and fixed up where necessary; thanks for the heads-up on view tests!
--
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 #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-739818282
Long thread :) - I think however that one is not critical and can be done in 2.1 ? @ashb @ryanahamilton WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-955108001
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r591542423
##########
File path: airflow/www/views.py
##########
@@ -449,6 +490,8 @@ def get_int_arg(value, default=0):
arg_search_query = request.args.get('search')
arg_tags_filter = request.args.getlist('tags')
arg_status_filter = request.args.get('status')
+ arg_sorting_key = request.args.get('sortBy')
+ arg_orderby_key = request.args.get('orderBy')
Review comment:
K, sortBy and orderBy as argument names is too confusing.
I'd like us to either:
a) change to the single column using `-` (`order_by=col` for asc, `order_by=-col` for descending.); or
b) change orderBy to `orderDir` (have I got this the right way around?)
a has the advantage that you could then sort by multiple columns (`order_by=col1,-col2`) for instance but I'd be okay not supporting that 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r511151210
##########
File path: airflow/www/utils.py
##########
@@ -302,6 +311,48 @@ def dag_run_link(attr):
'<a href="{url}">{run_id}</a>').format(url=url, run_id=run_id) # noqa
+def dag_query_for_key(sorting_key):
+ """Maps sorting key param in the URL params to DB queries on appropriate attributes."""
+ dag_query_key_map = {
+ 'dag_id': DagModel.dag_id,
+ 'owner': DagModel.owners,
+ 'schedule': DagModel.next_dagrun,
+ # <- add any extra (URL param)->(DagModel attr) mappings here
+ }
Review comment:
It might be easier/less code to have a single `sort_by` and have this as
```suggestion
dag_query_key_map = {
'dag_id': DagModel.dag_id,
'-dag_id': DagModel.dag_id.desc(),
'owner': DagModel.owners,
'-owner': DagModel.owners.desc(),
'schedule': DagModel.next_dagrun,
'-schedule': DagModel.next_dagrun.desc(),
# <- add any extra (URL param)->(DagModel attr) mappings here
}
```
That way we don't need to have extra sorting_order param. I think this makes the code and the URI simpler. (For instance, `query_ordering_transform_for_key` wouldn't be needed anymore)
Additionally this order probably doesn't belong in this file -- but instead in the views.py file, probably in the `home` function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r518387513
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,51 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering == 'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') != attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}">
+ {{ name }}
+ <span id="sorting-tip-{{ name }}" class="tooltip" role="tooltip" >
+ {% if curr_ordering == 'unsorted' %}
+ Column not currently sorted.
+ {% else %}
+ Column sorted by {{ attr_name }}, {{ verbose_ordering_name(curr_ordering) }}.
+ {% endif %}
+ Press to sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}.
+ </span>
+ </a>
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}">
+ <!-- We need a separate <a> here because otherwise some screen readers STILL pick it up for some reason >_< -->
Review comment:
Curious about this… the extra anchor seems like a lot of extra markup overhead. I just tested this my self using ChromeVox Classic Extension. The `aria-hidden="true"` was working as expected.
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,51 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering == 'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') != attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}">
+ {{ name }}
+ <span id="sorting-tip-{{ name }}" class="tooltip" role="tooltip" >
Review comment:
You can simply this and circumvent adding this tooltip span by adding a `class="js-tooltip"` and a `title="{{ tooltip }}"` attributes to the parent anchor.
IMO, the current tooltip text is a bit too verbose. I would suggest dropping the "Column not currently sorted." and "Column sorted by…" all together and just provide description of what clicking on will do: "Sort by ascending owner" etc.
```jinja
<a
…
class="js-tooltip"
title="Sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}"
>
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-721445290
Can you please rebase your PR on latest Master since we have applied [Black](https://github.com/apache/airflow/commit/4e8f9cc8d02b29c325b8a5a76b4837671bdf5f68) and [PyUpgrade](https://github.com/apache/airflow/commit/8c42cf1b00c90f0d7f11b8a3a455381de8e003c5) on Master.
It will help if your squash your commits into single commit first so that there are less conflicts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-732220999
[The Workflow run](https://github.com/apache/airflow/actions/runs/379178074) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-728798351
[The Workflow run](https://github.com/apache/airflow/actions/runs/367766644) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-729021777
[The Workflow run](https://github.com/apache/airflow/actions/runs/368443647) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-711855619
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] scrdest commented on a change in pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
scrdest commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r509259750
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -76,9 +76,21 @@ <h2>DAGs</h2>
<th width="12">
<span id="pause_header" class="material-icons text-muted" title="Use this toggle to pause a DAG. The scheduler won't schedule new tasks instances for a paused DAG. Tasks already running at pause time won't be affected.">info</span>
</th>
- <th>DAG</th>
- <th>Schedule</th>
- <th>Owner</th>
+ <th>
+ <a href="{{ url_for('Airflow.index', status=request.args.get('status', 'all'), search=request.args.get('search', None), tags=request.args.get('tags', None), sortBy='dag_id', orderBy='asc' if request.args.get('sortBy') != 'dag_id' or request.args.get('orderBy', 'desc') != 'asc' else 'desc' ) }}">
+ DAG
Review comment:
Added; will add screenshots to the PR body.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#issuecomment-739876748
Marked it for 2.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #11652:
URL: https://github.com/apache/airflow/pull/11652
--
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] closed pull request #11652: Support for sorting DAGs in the web UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #11652:
URL: https://github.com/apache/airflow/pull/11652
--
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