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 2022/01/02 22:48:24 UTC
[GitHub] [airflow] khalidmammadov opened a new pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
khalidmammadov opened a new pull request #20625:
URL: https://github.com/apache/airflow/pull/20625
Part of https://github.com/apache/airflow/issues/19891
- Fixed type of a field in a sub-class as super class was changed
- Applied `tuple` conversion to adhere to the type of the field: `Sequence[str]`
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777406294
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
This field is defined in the base class as `Sequence`. It used to be `Iterable` and was changed [here](https://github.com/apache/airflow/commit/59e4b78daa3496cb0358ce34aeb5ebf6f5565ce0#diff-848f325ace55b3504e8052fecdb53c0f295c891b67a6d90e9341cbe79cc545fbR428). I changed it to match base class and not sure if order here metters at all. BTW, it was suggested by you :) https://github.com/apache/airflow/pull/20034#discussion_r762426218
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777283387
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
Maybe this should have been `Collection` instead of `Sequence`? Or does anything in Airflow requires these to be deterministically ordered?
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777565405
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
If we choose another protocol - fine with me. That''s yet andother 280 files change :) .
Anyone happy to do it ?
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777427425
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
So better not change it again - unless we have a good reson to.
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777490279
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
The reason being the `tuple` change below indicating `Sequence` (which `set` does not implement) is potentially the wrong protocol.
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777826299
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
I’d say let’s keep `Sequence` and do the mass-change later. That makes the commit history easier to reason with.
--
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] khalidmammadov commented on a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777718335
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
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?
--
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] khalidmammadov commented on a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777518038
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
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:
https://github.com/apache/airflow/blob/84a4bcca293ac1c85a91dfc7f97938ff2fd9c564/airflow/providers/qubole/operators/qubole_check.py#L108-L110
Well, it also means that if we dont need dups then Set type would be more appropriate
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777426463
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
Yep. I already implemented Sequence for ALL operators in #20571 :).
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777427084
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
And #20608
--
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 a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777427425
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
So better not change it again - unless we have a good reason to.
--
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] khalidmammadov commented on a change in pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#discussion_r777713562
##########
File path: airflow/providers/qubole/operators/qubole.py
##########
@@ -217,7 +217,7 @@ class QuboleOperator(BaseOperator):
'cluster_label',
)
- template_ext: Iterable[str] = ('.txt',)
+ template_ext: Sequence[str] = ('.txt',)
Review comment:
I can change them to `Set` type if we are in an agreement?
--
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] kazanzhy commented on pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #20625:
URL: https://github.com/apache/airflow/pull/20625#issuecomment-1004382236
Looks good. Here the first try to fix Qubole
https://github.com/apache/airflow/pull/20319
--
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 merged pull request #20625: Fxing MyPy issues inside airflow/providers/qubole
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20625:
URL: https://github.com/apache/airflow/pull/20625
--
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