-
Notifications
You must be signed in to change notification settings - Fork 16.4k
allows users to write dag_id and task_id in their national characters, added display name for dag / task #32520
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
Conversation
|
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/main/CONTRIBUTING.rst)
|
|
Please set up |
airflow/migrations/versions/0123_2_5_0_add_display_name_for_dag_and_task_.py
Outdated
Show resolved
Hide resolved
|
Could you add an example dag so that we can test it out? |
Added |
|
Do we also want a unit test? (Open question; there’s not much to be tested tbh, most things are just declarative.) |
|
Seems that have https://github.com/apache/airflow/actions/runs/5687937364/job/15417962420?pr=32520 |
|
Looks like |
Yeah probably. And |
|
Sounds right to me |
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this approach, LGTM!
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a deployment and faced a couple of "glitches". Can you check and rework on these? Otherwise as expressed in the comments I really like the feature and would like to have this merged ASAP!
- I see many screens, w.g. Grid view not picking up the cool display names. I asuspect a bit of rework in the UI is needed. Low hanging fruit would be the DAG page header - if you could add using display name there would be great.
- Starting the system with breeze raised an import error with two examples. Must be fixed, otherwise I assume also a lot of DAGs "in the wild" will fail in there
- Two example DAGs are showing import errors when starting the system, probably the same root cause: "error DAG Import Errors (2)"
Error trace for 2)
ERROR [airflow.models.dagbag.DagBag] Failed to import: /opt/airflow/airflow/example_dags/example_dynamic_task_mapping.py
Traceback (most recent call last):
File "/opt/airflow/airflow/models/dagbag.py", line 345, in parse
loader.exec_module(new_module)
File "<frozen importlib._bootstrap_external>", line 843, in exec_module
File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
File "/opt/airflow/airflow/example_dags/example_dynamic_task_mapping.py", line 37, in <module>
added_values = add_one.expand(x=[1, 2, 3])
File "/opt/airflow/airflow/decorators/base.py", line 410, in expand
return self._expand(DictOfListsExpandInput(map_kwargs), strict=False)
File "/opt/airflow/airflow/decorators/base.py", line 478, in _expand
operator = _MappedOperator(
TypeError: __init__() missing 1 required keyword-only argument: 'task_display_name'
ERROR [airflow.models.dagbag.DagBag] Failed to import: /opt/airflow/airflow/example_dags/example_params_trigger_ui.py
Traceback (most recent call last):
File "/opt/airflow/airflow/models/dagbag.py", line 345, in parse
loader.exec_module(new_module)
File "<frozen importlib._bootstrap_external>", line 843, in exec_module
File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
File "/opt/airflow/airflow/example_dags/example_params_trigger_ui.py", line 103, in <module>
english_greetings = generate_english_greeting.expand(name=names)
File "/opt/airflow/airflow/decorators/base.py", line 410, in expand
return self._expand(DictOfListsExpandInput(map_kwargs), strict=False)
File "/opt/airflow/airflow/decorators/base.py", line 478, in _expand
operator = _MappedOperator(
TypeError: __init__() missing 1 required keyword-only argument: 'task_display_name'
Database migrating done!
Error trace for 3)
error DAG Import Errors (2) expand_less
expand_moreBroken DAG: [/opt/airflow/airflow/example_dags/example_dynamic_task_mapping.py] Traceback (most recent call last):
File "/opt/airflow/airflow/decorators/base.py", line 410, in expand
return self._expand(DictOfListsExpandInput(map_kwargs), strict=False)
File "/opt/airflow/airflow/decorators/base.py", line 478, in _expand
operator = _MappedOperator(
TypeError: __init__() missing 1 required keyword-only argument: 'task_display_name'
expand_moreBroken DAG: [/opt/airflow/airflow/example_dags/example_params_trigger_ui.py] Traceback (most recent call last):
File "/opt/airflow/airflow/decorators/base.py", line 410, in expand
return self._expand(DictOfListsExpandInput(map_kwargs), strict=False)
File "/opt/airflow/airflow/decorators/base.py", line 478, in _expand
operator = _MappedOperator(
TypeError: __init__() missing 1 required keyword-only argument: 'task_display_name'
DAGs
|
I tried to add a commit to address the error mentioned above, but there are multiple linting errors here and a merge conflict that must be fixed first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also want to update the react part (Grid, Cluster Activity, Dataset) to use this new display_name instead of the current task_id, dag_id
|
I'm resuming working on this - fix the issues and also change UI part. |
I resolved the conflicts and rebased the branch |
I will try on this but I’m surely not a frontend expert. |
No worries, try your best. If you feel like this is too much, I think we can do that in follow up PRs, step by step. cc: @bbovenzi |
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this PR and am looking forward getting this merged. I feel like there are probably another 100 places where the DAG would benefit from displaying the "display name" and not the ID. Might be able to have this updated in the UI incrementally as well to get this merged.
Can you fix-up the DB migration (I hope I understood the alembic tooling right, with the commented fixes it was working for a local test - I hope you don't need to fully re-generate
airflow/migrations/versions/0132_2_8_0_add_display_name_for_dag_and_task_.py
Outdated
Show resolved
Hide resolved
airflow/migrations/versions/0132_2_8_0_add_display_name_for_dag_and_task_.py
Outdated
Show resolved
Hide resolved
airflow/migrations/versions/0132_2_8_0_add_display_name_for_dag_and_task_.py
Outdated
Show resolved
Hide resolved
@uranusjr In general the fields on Can you give me some hints on how to achieve this? |
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Aleksandr Shelukheev <shelukheev@gmail.com>
|
I'd really love to have this in - 2.9.0 release cut is across the corner - possible to get the conflicts resolved, the final bug fixed and have this merged? |
|
Hopefully I can spend some time on this maybe next week. |
|
As linked in #38446 - I tried to spend time on the weekend to continue working on this PR... let's see if we can get this reviewed and completed... |
ephraimbuddy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
| else: | ||
| return self.node_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| else: | |
| return self.node_id | |
| return self.node_id |
|
Closing this PR as completed with #38446 - thanks for the contribution on this branch which was the base for the efforts to make it in (hopefully, if all runs fine) Airflow 2.9.0! |
This is picking up the work from #28183. But do not silently slugify the IDs