You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jose-lpa (via GitHub)" <gi...@apache.org> on 2023/02/25 09:37:30 UTC
[GitHub] [airflow] jose-lpa opened a new pull request, #29760: Issue-29759: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
jose-lpa opened a new pull request, #29760:
URL: https://github.com/apache/airflow/pull/29760
<!--
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: #29759
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] jose-lpa commented on a diff in pull request #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "jose-lpa (via GitHub)" <gi...@apache.org>.
jose-lpa commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117921298
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I'm sincerely sorry that this PR was bothering you instead of helping or adding any improvement at all, which was my only intention.
> _Generally, you should only bother to optimize code if you really need to_
I have read that, and I would reply to him that if somebody else comes to my codebase and proves that a single function could be significantly improved with a very simple change, I will just follow the advice. Because that is very different than **me** _bothering to optimize code_ (read: "spending my own time reviewing the entire codebase and **trying** to find improvements"). It's definitely a different scenario: it's someone helping to improve my code for free.
I thought that the improvement was clear and simple enough to be considered, as it's so evident that the current implementation is performing multiple extra operations that could be dismissed, and adding instead a loop to iterate over multiple cases is definitely not the best choice - you can do that with `elif`'s and don't need to do any loop.
Finally, I understand being a maintainer of such big project is not easy. This PR was just trying to contribute somehow to open source on my free time - I saw the code before this week, trying to debug some issue I had. I'm not here to argue or waste my (and **yours**) time with discussions. You are the person in charge and the last one to decide, not me. I've explained enough my PR. If it's not appreciated, or is causing more trouble than help, just close the PR and the issue and that's all. I won't be pushing for it anymore.
Apologies again if you were annoyed for anything I said before.
Cheers, have a good weekend.
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117921522
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I am absolutely not annoyed. It's perfectly normal to get reviews that are proposing changes. to proposed PR. I think you take it far too personal.
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I am absolutely not annoyed. It's perfectly normal to get reviews that are proposing changes to proposed PR. I think you take it far too personal.
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29760:
URL: https://github.com/apache/airflow/pull/29760
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117897439
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
Technically speaking it is not equivalent. Isinstance will also work in case the object is derived from the type it checks. Not sure if that might happen here, but I think it would be better to have array of tuples and run for loop and run isinstance for each element. Smth like:
```
for _type, _template_fields in [(k8s.V1EnvVar, ("value", "name"), ....]:
if isinstance(content, _type):
template_fields = _template_fields
```
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117904897
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I'd argue that efficency in this case has very little importance. Even the SO answer you pointed out has this very statement (which I generally very much agree with - to an extent):
> Generally, you should only bother to optimize code if you really need to, i.e. if the program's performance is unusably slow.
Having a for loop with array of `type -> "fields" tuples does not seem like a bad idea (neither from performance POV nor clarity). Especially in Python 3.11 (which we hopefully support) with Specialized Adaptive Interpreter, those are precisely the kinds of optimizations that Python Interpreter will be able to optimize very efficiently.
I am simply extremely cautious with backwards-compatibility. I am ok with either approach if others, who get more k8s experience and defined the k8s configuration scheme. Maybe there are cases when those entities are extended. cc: @dstandish @dimberman
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117904897
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I'd argue that efficency in this case has very little importance. Even the SO answer you pointed out has this very statement (which I generally very much agree with - to an extent):
> Generally, you should only bother to optimize code if you really need to, i.e. if the program's performance is unusably slow.
Having a for loop with array of `type -> "fields" tuples does not seem like a bad idea (neither from performance POV nor clarity). Especially in Python 3.11 (which we hopefully support) with Specialized Adaptive Interpreter, those are precisely the kinds of optimizations that Python Interpreter will be able to optimize very efficiently.
I am simply extremely cautious with backwards-compatibility. I am ok with either approach if others, who get more k8s experience and defined the k8s configuration scheme confirm that it's very unlikely to get those entities extended. cc: @dstandish @dimberman
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117905335
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
BTW. @jose-lpa I am pretty sure your point of view would be different if you get ot answer up to 30 issues a day of people whos deployment suddenly stopped working afer upgrade. As maintainers we have to deal with those cases and you as an author usually spare very little thought on that. This is why our perspectives are different and understanding the perspective of maintainer is a key for a good contribution.
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117921522
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I am absolutely not annoyed. It's perfectly normal to get reviewes that are proposing changes. to proposed PR. I think you take it far too personal.
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117921546
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
(look at the other PRs out there and see yourself how it works here).
--
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] boring-cyborg[bot] commented on pull request #29760: Issue-29759: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #29760:
URL: https://github.com/apache/airflow/pull/29760#issuecomment-1445041440
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] boring-cyborg[bot] commented on pull request #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #29760:
URL: https://github.com/apache/airflow/pull/29760#issuecomment-1447617202
Awesome work, congrats on your first merged pull request!
--
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] jose-lpa commented on a diff in pull request #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "jose-lpa (via GitHub)" <gi...@apache.org>.
jose-lpa commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1118641912
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
Thank you both, @potiuk @hussein-awala.
I changed the code to `elif` conditionals as suggested which will also prevent to re-check the `content` value more than it is needed, while giving the users flexibility to extend K8s library classes if they need 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] potiuk commented on a diff in pull request #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117978501
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
That will do as well.
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117904897
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I'd argue that efficency in this case has very little importance. Even the SO answer you pointed out has this very statement (which I generally very much agree with):
> Generally, you should only bother to optimize code if you really need to, i.e. if the program's performance is unusably slow.
Having a for loop with array of `type -> "fields" tuples does not seem like a bad idea (neither from performance POV nor clarity). Especially in Python 3.11 (which we hopefully support) with Specialized Adaptive Interpreter, those are precisely the kinds of optimizations that Python Interpreter will be able to optimize very efficiently.
I am simply extremely cautious with backwards-compatibility. I am ok with either approach if others, who get more k8s experience and defined the k8s configuration scheme. Maybe there are cases when those entities are extended. cc: @dstandish @dimberman
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117904897
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
I'd argue that efficency in this case has very little importance. Even the SO answer you pointed out has this very statement (which I generally very much agree with):
> Generally, you should only bother to optimize code if you really need to, i.e. if the program's performance is unusably slow.
Having a for loop with array of `type -> "fields" tuples does not seem like a bad idea (neither from performance POV). Especially in Python 3.11 (which we hopefully support) with Specialized Adaptive Interpreter, those are precisely the kinds of optimizations that Python Interpreter will be able to optimize very efficiently.
I am simply extremely cautious with backwards-compatibility. I am ok with either approach if others, who get more k8s experience and defined the k8s configuration scheme. Maybe there are cases when those entities are extended. cc: @dstandish @dimberman
--
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] jose-lpa commented on a diff in pull request #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "jose-lpa (via GitHub)" <gi...@apache.org>.
jose-lpa commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117900064
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
It is not equivalent, as you said, if you want to check **any** objects type, also the ones derived from the base type. But in our specific case I see this is not going to happen, so the hashing is a nice fit IMO, faster and cleaner code than conditionals or loops.
Take into account that the hashmap is a direct match, falling back to the default value in case it's a missed hit. For these few values that we have to match this pattern is very efficient, and you avoid looping or triggering conditionals which are more expensive to run.
If you need an example of the internal difference, [this SO thread has a very nice insight using `dis`](https://stackoverflow.com/a/15925086/515093).
--
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] jose-lpa commented on a diff in pull request #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "jose-lpa (via GitHub)" <gi...@apache.org>.
jose-lpa commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117900064
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
It is not equivalent, as you said, if you want to check **any** objects type, also the ones derived from the base type. But in our specific case I see this is not going to happen, so the hashing is a nice fit IMO, faster and cleaner code than conditionals or loops.
Take into account that the hashmap is a direct match, falling back to the default value in case it's a missed hit. For this few values we have to match this pattern is very efficient, and you avoid looping or triggering conditionals which are more expensive to run.
If you need an example of the internal difference, [this SO thread has a very nice insight using `dis`](https://stackoverflow.com/a/15925086/515093).
--
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 #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117905335
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
BTW. @jose-lpa I am pretty sure your point of view would be different if you get to answer up to 30 issues a day of people whos deployment suddenly stopped working afer upgrade. As maintainers we have to deal with those cases and you as an author usually spare very little thought on that. This is why our perspectives are different and understanding the perspective of maintainer is a key for a good contribution.
--
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] hussein-awala commented on a diff in pull request #29760: `KubernetesPodOperator._render_nested_template_fields` improved by changing the conditionals for a map
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29760:
URL: https://github.com/apache/airflow/pull/29760#discussion_r1117977100
##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -378,22 +378,16 @@ def _render_nested_template_fields(
seen_oids: set,
) -> None:
if id(content) not in seen_oids:
- template_fields: tuple | None = None
-
- if isinstance(content, k8s.V1EnvVar):
- template_fields = ("value", "name")
-
- if isinstance(content, k8s.V1ResourceRequirements):
- template_fields = ("limits", "requests")
-
- if isinstance(content, k8s.V1Volume):
- template_fields = ("name", "persistent_volume_claim")
-
- if isinstance(content, k8s.V1VolumeMount):
- template_fields = ("name",)
-
- if isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
- template_fields = ("claim_name",)
+ try:
Review Comment:
@jose-lpa Thank you for opening this PR!
I agree with @potiuk about the need to check if the object is derived from the type, where some people extend the k8s classes to define their own default values or simplifying the creating of its instances, ex:
```python
class CompanyResourceRequirements(k8s.V1ResourceRequirements):
def __init__(self, cpu, memory):
resources = {"cpu": cpu, "memory": memory}
super().__init__(limits=resources, requests=resources)
resources_requirements = CompanyResourceRequirements(cpu="1", memory="512m")
isinstance(resource_requirements, k8s.V1ResourceRequirements)
>>> True
```
Also we will not gain a lot because python needs resources to create the dictionary and create the hashes. Instead we can replace the `if` by `elif` and avoid the useless checks when we find the class of our instance:
```python
if isinstance(content, k8s.V1EnvVar):
template_fields = ("value", "name")
elif isinstance(content, k8s.V1ResourceRequirements):
template_fields = ("limits", "requests")
elif isinstance(content, k8s.V1Volume):
template_fields = ("name", "persistent_volume_claim")
elif isinstance(content, k8s.V1VolumeMount):
template_fields = ("name",)
elif isinstance(content, k8s.V1PersistentVolumeClaimVolumeSource):
template_fields = ("claim_name",)
```
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