[Webserver] Use operator_name instead of task_type in UI by utkarsharma2 · Pull Request #31662 · apache/airflow

utkarsharma2 · 2023-06-01T08:33:45Z

we should use operator_name when displaying operator name in UI.

Related: #28466 (not closing yet, see discussion below)

utkarsharma2 · 2023-06-01T10:48:29Z

Found one more instance of task_type usage in UI, working on changes accordingly.

utkarsharma2 · 2023-06-02T04:09:12Z

@uranusjr While going through the code I found there are two other tooltips where we show the operator name

airflow/airflow/www/static/js/task_instances.js

Line 111 in 29d2a31

tt += `Operator: ${escapeHtml(ti.operator)}<br>`;

and

airflow/airflow/www/static/js/task_instances.js

Line 170 in 29d2a31

tt += `Operator: ${escapeHtml(ti.operator)}<br>`;

Ideally, both of them should show values derived from custom_operator_name but only the base operator has the operator_name property and not the task instance object. In both of the instance as mentioned above, we are displaying values of ti.operator which are populated from task_type of operator.

airflow/airflow/models/taskinstance.py

Line 888 in 29d2a31

self.operator = task.task_type

since self.operator is getting used at multiple places in code, I think we should create another property in the task instance operator similar to operator_name in the base operator and use that while displaying in UI.

However, this change can be a separate PR. WDYT?

uranusjr · 2023-06-02T09:08:16Z

Hmm, yeah adding a new field is probably the simplest approach. Can you find other uses of TaskInstance.operator so maybe we can analyse if this field is simply for UI (if so, we can just change it to use operator_name), or do other logic depend on it? Adding a field is relatively expensive on TaskInstance so we should avoid that if possible.

Either way, I think we should hold this PR as-is and continue looking into TaskInstance.operator in a different PR.

uranusjr · 2023-06-02T09:11:04Z

@@ -3084,6 +3084,7 @@ class GraphForm(DateTimeWithNumRunsWithDagRunsForm):
            t.task_id: {
                "dag_id": t.dag_id,
                "task_type": t.task_type,


Do we still need this? If the only use case if to check SubDagOperator, we can use operator_name as well.

Yes, I think we still need this since the below code compares the operator class name and it's best to keep this since the operato_name can change.

airflow/airflow/www/static/js/graph.js

Line 211 in 8891c69

isSubDag: task.task_type === "SubDagOperator",

and based on that displaying the subdag button -

airflow/airflow/www/static/js/callModal.js

Lines 185 to 190 in 8891c69

if (isSubDag) {

$("#div_btn_subdag").show();

subDagId = `${dagId}.${taskId}`;

} else {

$("#div_btn_subdag").hide();

}

Wouldn't it be better to pass is_subdag from the webserver instead? That's what we do in the REST API

@bbovenzi Would it make sense to raise a separate PR for this change? because I think it's a separate task.

Yeah, let's do a separate PR.

utkarsharma2 · 2023-06-08T05:24:16Z

Hmm, yeah adding a new field is probably the simplest approach. Can you find other uses of TaskInstance.operator so maybe we can analyze if this field is simply for UI (if so, we can just change it to use operator_name), or do other logic depends on it? Adding a field is relatively expensive on TaskInstance so we should avoid that if possible.

Either way, I think we should hold this PR as-is and continue looking into the TaskInstance.operator in a different PR.

@uranusjr I checked for the usage of ti.operator and found out there are a lot of places where it's been used, below are a few examples

airflow/airflow/models/dagrun.py

Line 1126 in 8891c69

created_counts[ti.operator] += 1

airflow/airflow/models/dag.py

Line 1749 in 8891c69

for ti in query.filter(TI.operator == ExternalTaskMarker.__name__):

airflow/airflow/jobs/scheduler_job_runner.py

Line 744 in 8891c69

ti.operator,

TaskInstance.operator certainly has more usage than just displaying in UI.

I think adding another field to TaskInstance will be a quick solution. WDYT?

uranusjr · 2023-06-08T05:41:20Z

Yes that’s probably best. I think some of the use cases can technically switch to operator_name (e.g. logging), but at least the second one you mentioned should use the raw class name.

utkarsharma2 · 2023-06-10T21:12:27Z

@uranusjr Added a new property custom_operator_name in TaskInstance class.

Also, would be nice if you can cross verify all the places that should be populating the value for custom_operator_name are covered.

utkarsharma2 · 2023-06-11T05:28:28Z

@uranusjr since there is a new field added to TaskInstance class which requires a new column to be created in TaskInstance table. I think this would be a breaking change, right? Do we have something that does database migration similar to Django migrations? if so, we can add a default value so that the database migrations can be smooth for existing TaskInstance.

potiuk · 2023-06-11T10:29:49Z

@uranusjr since there is a new field added to TaskInstance class which requires a new column to be created in TaskInstance table. I think this would be a breaking change, right? Do we have something that does database migration similar to Django migrations? if so, we can add a default value so that the database migrations can be smooth for existing TaskInstance.

We have alembic. You can see the docs about addin new migrations (look for alembic) and you can also take a look at past prs that added migrations to airflow\migrations. Also worth looking at the airflow db command and all their subcommands and https://airflow.apache.org/docs/apache-airflow/stable/migrations-ref.html

jedcunningham · 2023-06-15T17:54:54Z

@@ -3084,6 +3084,7 @@ class GraphForm(DateTimeWithNumRunsWithDagRunsForm):
            t.task_id: {
                "dag_id": t.dag_id,
                "task_type": t.task_type,


Yeah, let's do a separate PR.

Use operator_name

801cb50

utkarsharma2 requested review from ashb, bbovenzi, pierrejeambrun and ryanahamilton as code owners June 1, 2023 08:33

boring-cyborg Bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jun 1, 2023

utkarsharma2 marked this pull request as draft June 1, 2023 08:33

utkarsharma2 changed the title ~~Use operator_name~~ [Webserver] Use operator_name instead of task_type in UI Jun 1, 2023

Merge branch 'main' into customOperatorName

240fbca

utkarsharma2 marked this pull request as ready for review June 1, 2023 08:36

uranusjr reviewed Jun 1, 2023

View reviewed changes

Comment thread airflow/www/views.py

Remove unwanted change

ab77a1d

utkarsharma2 requested a review from uranusjr June 1, 2023 09:00

uranusjr approved these changes Jun 1, 2023

View reviewed changes

utkarsharma2 marked this pull request as draft June 1, 2023 10:48

Update tooltip code with

b08e4d0

utkarsharma2 requested a review from uranusjr June 2, 2023 03:54

utkarsharma2 marked this pull request as ready for review June 2, 2023 03:55

Merge branch 'main' into customOperatorName

c3818f1

uranusjr reviewed Jun 2, 2023

View reviewed changes

eladkal added this to the Airflow 2.6.2 milestone Jun 2, 2023

eladkal modified the milestones: Airflow 2.6.2, Airlfow 2.6.3 Jun 8, 2023

utkarsharma2 requested review from XD-DENG, kaxil and potiuk as code owners June 12, 2023 04:00

utkarsharma2 force-pushed the customOperatorName branch from 149a8c9 to 3a4b580 Compare June 12, 2023 04:58

uranusjr reviewed Jun 12, 2023

View reviewed changes

Comment thread airflow/models/taskinstance.py Outdated

utkarsharma2 force-pushed the customOperatorName branch from 7ef85bd to c3818f1 Compare June 12, 2023 06:24

uranusjr approved these changes Jun 13, 2023

View reviewed changes

jedcunningham approved these changes Jun 15, 2023

View reviewed changes

jedcunningham merged commit 1950efe into apache:main Jun 15, 2023

ephraimbuddy modified the milestones: Airlfow 2.6.3, Airflow 2.7.0 Jul 6, 2023

ephraimbuddy added type:improvement Changelog: Improvements type:new-feature Changelog: New Features and removed type:improvement Changelog: Improvements labels Jul 6, 2023

	if (isSubDag) {
	$("#div_btn_subdag").show();
	subDagId = `${dagId}.${taskId}`;
	} else {
	$("#div_btn_subdag").hide();
	}

Uh oh!

Conversation

utkarsharma2 commented Jun 1, 2023 • edited by uranusjr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

utkarsharma2 commented Jun 1, 2023

Uh oh!

utkarsharma2 commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uranusjr commented Jun 2, 2023

Uh oh!

uranusjr Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

utkarsharma2 Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbovenzi Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utkarsharma2 Jun 10, 2023

Choose a reason for hiding this comment

Uh oh!

jedcunningham Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

utkarsharma2 commented Jun 8, 2023

Uh oh!

uranusjr commented Jun 8, 2023

Uh oh!

utkarsharma2 commented Jun 10, 2023

Uh oh!

utkarsharma2 commented Jun 11, 2023

Uh oh!

potiuk commented Jun 11, 2023

Uh oh!

Uh oh!

jedcunningham Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

utkarsharma2 commented Jun 1, 2023 •

edited by uranusjr

Loading

utkarsharma2 commented Jun 2, 2023 •

edited

Loading

utkarsharma2 Jun 8, 2023 •

edited

Loading

bbovenzi Jun 8, 2023 •

edited

Loading