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/06/27 04:21:14 UTC
[GitHub] [airflow] uranusjr opened a new pull request, #24673: Rename 'resources' arg in Kub op to k8s_resources
uranusjr opened a new pull request, #24673:
URL: https://github.com/apache/airflow/pull/24673
Using 'resources' to mean a different thing is incompatible with the base operator. It breaks Liskov substitution and results in incorrect task mapping behavior. Renaming the argument fixes everything.
Fix #23783.
--
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 pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1196224926
This is already in the latest apache-airflow-providers-cncf-kubernetes release.
--
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] savingoyal commented on a diff in pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
savingoyal commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907761806
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
Wouldn't this be a breaking change with a significant impact on user DAGs? Curious, what is the backward compatibility philosophy for such changes?
--
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 a diff in pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907795836
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
another option would be to accept `task_resources` and forward this to BaseOperator `resources` and keep `resources` to mean pod resources but probably no one will like this idea ;)
--
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 #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1179678290
We release providers roughly monthly - the next wave is coming in the next few days + min 3 days for voting. But the actual release time depends very much on testing i tby the users.
I will male sure to add you to the least of people whowwen i release the providers @valayDave .also that you can test it and confirm that it solves your problem.
Generally speaking the sooner people like you confirm that the relaese candidate is sound, the more likely it will be released faster
--
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] github-actions[bot] commented on pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1167224691
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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 #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r908003508
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
container_resources sounds good
--
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] valayDave commented on pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
valayDave commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1195954907
Hello, which release will this be a part of ?
--
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 pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1167263625
Yeah, right now `resources` is only used by `CgroupTaskRunner`. Maybe someone could implement some kind of interface to use `resources` with Kubernetes, but directly passing in a `V1ResourceRequirements` is not the way 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] valayDave commented on pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
valayDave commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1179620310
What version of the Kubernetes operator will incorporate this change? Any idea when this change will be incorporated into the public release?
--
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 diff in pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907984624
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
Let’s go with `container_resources` then.
@savingoyal Passing `resources` will continue to work, emitting a deprecation warning. You must use the new argument for task mapping, but that doesn’t work right now anyway so nothing is broken.
--
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 a diff in pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907883591
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
ah interesting yeah sounds good
--
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 merged pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #24673:
URL: https://github.com/apache/airflow/pull/24673
--
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 #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1167800469
wow i had no clue we had this thing
--
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 a diff in pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907795836
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
another option would be to accept `task_resources` and forward this to BaseOperator `resources`
--
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 #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907739432
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
Just something to think about
Do we want k8s_ or kubernetes_ ?
--
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 a diff in pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907794917
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
or what about `pod_resources`
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
or what about `pod_resources` -- this seems better
--
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] jedcunningham commented on a diff in pull request #24673: Rename 'resources' arg in Kub op to k8s_resources
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24673:
URL: https://github.com/apache/airflow/pull/24673#discussion_r907810626
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -112,7 +112,7 @@ class KubernetesPodOperator(BaseOperator):
:param annotations: non-identifying metadata you can attach to the Pod.
Can be a large range of data, and can include characters
that are not permitted by labels.
- :param resources: resources for the launched pod.
+ :param k8s_resources: resources for the launched pod.
Review Comment:
> or what about `pod_resources` -- this seems better
`container_resources` would be better (it is set on the container after all)
--
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] valayDave commented on pull request #24673: Rename 'resources' arg in Kub op to container_resources
Posted by GitBox <gi...@apache.org>.
valayDave commented on PR #24673:
URL: https://github.com/apache/airflow/pull/24673#issuecomment-1198755381
This is not in the latest documentation hence I asked. I see docs only until 4.2.0 for the kuberentes provider.
--
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