KubernetesPodOperator._render_nested_template_fields improved by changing the conditionals for a map

[jose-lpa]

Closes: #29759

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

[boring-cyborg]

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 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 Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
  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

[potiuk]

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

[jose-lpa]

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.

[potiuk]

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

[potiuk]

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.

[jose-lpa]

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.

[potiuk]

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.

[potiuk]

(look at the other PRs out there and see yourself how it works here).

[hussein-awala]

@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:

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:

            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

[potiuk]

That will do as well.

[jose-lpa]

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.

[hussein-awala]

LGTM

[boring-cyborg]

Awesome work, congrats on your first merged pull request!