You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "eladkal (via GitHub)" <gi...@apache.org> on 2023/03/03 20:21:51 UTC
[GitHub] [airflow] eladkal opened a new pull request, #29905: Align cncf provider file names with AIP-21
eladkal opened a new pull request, #29905:
URL: https://github.com/apache/airflow/pull/29905
Changes to import paths:
`operators.kubernetes_pod` -> `operators.pod`
`triggers.kubernetes_pod` -> `triggers.pod`
This PR is backward compatible
<!--
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 an 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/
-->
---
**^ 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 changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] dstandish commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1549190593
> > I am confused what was the basis of this change. I thought AIP-21 just said that you should omit the _operator suffix. Why remove the kubernetes_? We still keep it (and very much need it), for example, on the kubernetes hook?
>
> Please explain why this is needed? All providers are aligned without the prefix this is the convention.
My sentence structure maybe makes it harder to see but to clarify I'm talking about with kubernetes hook we call the file `kubernetes.py` -- what would you call it if you removed the "kubernetes"? That's what I mean. :)
> > I should clarify, specifically I'm concerned with the rename of kubernetes_pod.py > pod.py
>
> Please clarify why. We are not going to remove kubernetes_pod.py any time soon.. This is one that will stay with us for long till we are fully confident that most users migrated. At least 1 year to my taste.. even more (at least this is how I see it)
When I wrote "specifically I'm concerned" I was clarifying that that was the rename I was asking about because there were other renames in the PR but I was just asking about pod.py (initially i said "what's the reason for this change" so it wasn't clear what I was asking about. I'm not saying that the change is like... especially "concerning" just clarifying what I'm asking about.
But yeah so anyway, just not sure why the change and curious to understand, does not seem that it is required / suggested / indicated by the AIP. I thought that `kubernetes_pod.py` was the right name -- just chopping the `_operator` suffix.
--
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] eladkal merged pull request #29905: Align cncf provider file names with AIP-21
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #29905:
URL: https://github.com/apache/airflow/pull/29905
--
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] dstandish commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1550119067
> The import is `from airflow.providers.cncf.kubernetes.operators.pod import KubernetesPodOperator`
>
> You know it's Kubernetes because you import it from Kubernetes provider. There is no reason to have `kubernetes_` for all `py` file in the provider.
Yea I guess my interpretation of AIP-21 was not that we avoid all redundancy (and hooks is a good example where we don't do this, because then your file is `_.py`) but a rule about how to map to module name from class name. So FtpHook becomes ftp.py, KubernetesPodOperator becomes kubernetes_pod.py -- and indeed, someone in the past must have also shared this interpretation when it was originally renamed from `kubernetes_pod_operator.py` to `kubernetes_pod.py`.
I see the logic, that from organization perspective, the information ("kubernetes") is there in the module path, so in that sense you can say it's not necessary. I guess I'm with Jarek in preferring the verbosity -- and like he was mentioning, it's nice for finding a file, e.g.:
<img width="742" alt="image" src="https://github.com/apache/airflow/assets/15932138/2db791b5-9a34-4c06-9c2e-8c4f216b2344">
vs
<img width="742" alt="image" src="https://github.com/apache/airflow/assets/15932138/7f49ffb2-1816-4103-bd84-5259d95af055">
But I am also with Jarek in the sense of not standing in the way of the shorter naming if that's what the community prefers.
--
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] o-nikolas commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1454145484
Some static checks need fixing and warnings need accounting for, but otherwise the changes lgtm :+1:
--
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] eladkal commented on a diff in pull request #29905: Align cncf provider file names with AIP-21
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29905:
URL: https://github.com/apache/airflow/pull/29905#discussion_r1125517656
##########
airflow/providers/cncf/kubernetes/provider.yaml:
##########
@@ -89,6 +89,7 @@ operators:
- integration-name: Kubernetes
python-modules:
- airflow.providers.cncf.kubernetes.operators.kubernetes_pod
+ - airflow.providers.cncf.kubernetes.operators.pod
Review Comment:
Thanks for opening https://github.com/apache/airflow/issues/29918
--
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] dstandish commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1548388074
I should clarify, specifically I'm concerned with the rename of `kubernetes_pod.py` > `pod.py`
--
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 diff in pull request #29905: Align cncf provider file names with AIP-21
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29905:
URL: https://github.com/apache/airflow/pull/29905#discussion_r1125497225
##########
airflow/providers/cncf/kubernetes/provider.yaml:
##########
@@ -89,6 +89,7 @@ operators:
- integration-name: Kubernetes
python-modules:
- airflow.providers.cncf.kubernetes.operators.kubernetes_pod
+ - airflow.providers.cncf.kubernetes.operators.pod
Review Comment:
We do not have triggers in provider info - but that's a good idea to add them (we would also need to expand the CLI and provider manager to include and expose that information).
--
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] eladkal commented on a diff in pull request #29905: Align cncf provider file names with AIP-21
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29905:
URL: https://github.com/apache/airflow/pull/29905#discussion_r1125496510
##########
airflow/providers/cncf/kubernetes/provider.yaml:
##########
@@ -89,6 +89,7 @@ operators:
- integration-name: Kubernetes
python-modules:
- airflow.providers.cncf.kubernetes.operators.kubernetes_pod
+ - airflow.providers.cncf.kubernetes.operators.pod
Review Comment:
btw @potiuk triggers are part of providers but I noticed we don't specify them as part of the provider yaml. Is this expected?
--
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] eladkal commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1549228093
> My sentence structure maybe makes it harder to see but to clarify I'm talking about with kubernetes hook we call the file kubernetes.py -- what would you call it if you removed the "kubernetes"? That's what I mean. :)
The import is
`from airflow.providers.cncf.kubernetes.operators.pod import KubernetesPodOperator`
You know it's Kubernetes because you import it from Kubernetes provider. There is no reason to have `kubernetes_` for any `py` file in the provider.
> When I wrote "specifically I'm concerned" I was clarifying that that was the rename I was asking about because there were other renames in the PR but I was just asking about pod.py (initially i said "what's the reason for this change" so it wasn't clear what I was asking about). I'm not saying that the change is like... especially "concerning" just clarifying what I'm asking about.
When we have single hook we normally name it after the provider itself.
```
from airflow.providers.asana.hooks.asana import AsanaHook
from airflow.providers.ftp.hooks.ftp import FTPHook
from airflow.providers.http.hooks.http import HttpHook
```
When we have multiple hooks we name it after the service (like you can see in Amazon and Google providers.
About operators note that https://github.com/apache/airflow/pull/29930 is going to add
`airflow.providers.cncf.kubernetes.operators.resource`
so having
`airflow.providers.cncf.kubernetes.operators.pod`
along side make sense to me.
We are not quite there in having this completely organized, we probably need better defined policy with enforcement to bind all providers.
--
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] eladkal commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1549167414
> I am confused what was the basis of this change. I thought AIP-21 just said that you should omit the _operator suffix. Why remove the kubernetes_? We still keep it (and very much need it), for example, on the kubernetes hook?
Please explain why this is needed?
All providers are aligned without the prefix this is the convention.
> I should clarify, specifically I'm concerned with the rename of kubernetes_pod.py > pod.py
Please clarify why. We are not going to remove kubernetes_pod.py any time soon.. This is one that will stay with us for long till we are fully confident that most users migrated. At least 1 year to my taste.. even more.
--
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 #29905: Align cncf provider file names with AIP-21
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1550131558
Well. If there are two of us, we might actually attempt to rever that decision..... :exploding_head:
--
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] dstandish commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1548385196
I am confused what was the basis of this change. I thought AIP-21 just said that you should omit the _operator suffix. Why remove the kubernetes_? We still keep it (and very much need it), for example, on the kubernetes hook?
--
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 #29905: Align cncf provider file names with AIP-21
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1454730324
(Pending fixing the 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] eladkal commented on a diff in pull request #29905: Align cncf provider file names with AIP-21
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29905:
URL: https://github.com/apache/airflow/pull/29905#discussion_r1125496510
##########
airflow/providers/cncf/kubernetes/provider.yaml:
##########
@@ -89,6 +89,7 @@ operators:
- integration-name: Kubernetes
python-modules:
- airflow.providers.cncf.kubernetes.operators.kubernetes_pod
+ - airflow.providers.cncf.kubernetes.operators.pod
Review Comment:
btw @potiuk triggers are part of providers but I noticed we don't specify them as part of the provider yaml. Is this expected? As part of this change I had to specify only the new operator but not the new trigger. the old one also not listed.
--
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 #29905: Align cncf provider file names with AIP-21
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1549224491
I think (and this is why I approve it) - this change was really going together with the "spirit" of the AIP 21 even if not necessarily the "letter" of it.
The spirit of AIP-21 was that we remove redundant, repeated names from the full path of the class/module. That was the gist of it and the reason we implemented the change.
And In this case. the change was rather following that:
```
from airflow.providers.cncf.kubernetes.operators.kubernetes_pod import KubernetesPodOperator
```
contains `kubernetes` 3 times.
To be perfectly honest, I have not agreed with the change when we implemented it. I was against shortening the imports because I though that rather than length of the imports, the important thing is that when I press SHIFT (search anywhere) three times in my IntelliJ IDE and start typing `datadog` I can see `datadog_operators.py`, `datadog_hooks.py`, `datadog_sensors.py` rather than three `datadog.py` so that I can choose the one I want easier. It was made easier (actually during our discussion back then) that Intellij started to also show PATH in the result of the search so I did not oppose that much and went into "disagree but engage" mode (but it somewhat haunts me till today even though it is pretty usable even now with those shorter names):
I **think** this is just a natural application to be more consistent there. Again - I would personally prefer to go with the `kubernetes_` prefix but the spirit of AIP-21 was to optimise the import length and reduce redundancy and for consistency, I think it makes sense to use the `pod.py`
--
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] eladkal commented on pull request #29905: Align cncf provider file names with AIP-21
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #29905:
URL: https://github.com/apache/airflow/pull/29905#issuecomment-1550306966
AIP-21 did not completely resolved all issues.
For example:
We have `AutoMLPredictOperator`, `BigtableCreateInstanceOperator` in Google provider
We have EC2TerminateInstanceOperator, GlueJobOperator in Amazon provider
Then why do we have `KubernetesPodOperator`? Should it be `PodOperator`? I'm not sure.
I can show other examples that we do have the name of the provider as part of the operator name.
I can raise like 5-6 more issues of inconstancy of how we name things. We can reopen this and potentially come up with a replacement to AIP-21 with new rules and some enforcements but that is a massive refactor.
--
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