Skip to content

Fxing MyPy issues inside airflow/providers/qubole #20625

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.

Sign up for GitHub

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

Merged
potiuk merged 1 commit into from
Jan 4, 2022
+3 −3
Merged

Fxing MyPy issues inside airflow/providers/qubole #20625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions airflow/providers/qubole/operators/qubole.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"""Qubole operator"""
import re
from datetime import datetime
from typing import TYPE_CHECKING, Iterable, Optional, Sequence
from typing import TYPE_CHECKING, Optional, Sequence

from airflow.hooks.base import BaseHook
from airflow.models import BaseOperator, BaseOperatorLink
Expand Down Expand Up @@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)

template_ext: Iterable[str] = ('.txt',)
template_ext: Sequence[str] = ('.txt',)

@uranusjr uranusjr Jan 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe this should have been Collection instead of Sequence? Or does anything in Airflow requires these to be deterministically ordered?

@khalidmammadov khalidmammadov Jan 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This field is defined in the base class as Sequence. It used to be Iterable and was changed here. I changed it to match base class and not sure if order here metters at all. BTW, it was suggested by you :) #20034 (comment)

@potiuk potiuk Jan 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep. I already implemented Sequence for ALL operators in #20571 :).

@potiuk potiuk Jan 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And #20608

@potiuk potiuk Jan 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So better not change it again - unless we have a good reason to.

@khalidmammadov khalidmammadov Jan 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason for below set-tuple conversion is to remove any duplicates and hence set with union operator was used prior to conversion. It's similar to one in here:

airflow/airflow/providers/qubole/operators/qubole_check.py

Lines 108 to 110 in 84a4bcc

template_fields: Sequence[str] = tuple(
set(QuboleOperator.template_fields) | set(SQLCheckOperator.template_fields)
)

Well, it also means that if we dont need dups then Set type would be more appropriate

@potiuk potiuk Jan 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we choose another protocol - fine with me. That''s yet andother 280 files change :) .

Anyone happy to do it ?

@khalidmammadov khalidmammadov Jan 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can change them to Set type if we are in an agreement?

@khalidmammadov khalidmammadov Jan 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As a caution I will also need to change the type of values it (template_fields) gets assigned to, which are mostly tuple and will become all set. Also documentation needs to be amended as there were no much emphasis on the type of these fields and list also were used: https://airflow.apache.org/docs/apache-airflow/stable/concepts/operators.html#jinja-templating
This means we also need to notify users about this change.
Alternatively, we can leave them as is and let users to use list, tuple etc. to provide simplicity of use and internally carry on removing duplicates as they (dups) dont make any sense. WDYT?

@uranusjr uranusjr Jan 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I’d say let’s keep Sequence and do the mass-change later. That makes the commit history easier to reason with.

ui_color = '#3064A1'
ui_fgcolor = '#fff'
qubole_hook_allowed_args_list = ['command_type', 'qubole_conn_id', 'fetch_logs']
Expand Down
2 changes: 1 addition & 1 deletion airflow/providers/qubole/operators/qubole_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class QuboleValueCheckOperator(_QuboleCheckOperatorMixin, SQLValueCheckOperator,
QuboleOperator and SQLValueCheckOperator are template-supported.
"""

template_fields = set(QuboleOperator.template_fields) | set(SQLValueCheckOperator.template_fields)
template_fields = tuple(set(QuboleOperator.template_fields) | set(SQLValueCheckOperator.template_fields))
template_ext = QuboleOperator.template_ext
ui_fgcolor = '#000'

Expand Down