Move TaskInstanceKey to a separate file to fix circular import from kubernetes_helper_functions.py

[LipuFei]

Fixes #30988

Fixes a circular import of DagRun -> TaskInstance -> Sentry -> KubernetesExexcutor -> kubernetes_helper_functions.py (*) -> TaskInstanceKey.

Moving TaskInstanceKey out to a separate file will break the cycle from kubernetes_helper_functions.py -> taskinstances.py

[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: [email protected]
  Slack: https://s.apache.org/airflow-slack

[potiuk]

Nice. I like it - this is a simple solution and I think It is also generally a practice we are implementing more and more - splitting modules in order to get rid unnecessary coupling - especially with introducing more type hinting, some of the implicit couplings have been made explicit - like this one.

One thing to add though: https://github.com/apache/airflow/blob/main/docs/conf.py#L248 should also include taskinstancekey and it should also be added in https://github.com/apache/airflow/blob/main/docs/apache-airflow/public-airflow-interface.rst nest to taskinstance package as they are part of the public Airflow interface.

I also think it is backwards-compatible since TaskInstanceKey is imported in taskinstance.py (so if someone imports it from there, it will continue to work).

I would love another maintainer opinion, but it looks good to me.

[potiuk]

Another problem with that is that it causes import errors when collecting tests - it has to be fixed.

[potiuk]

And you will have to rebase to latest main @LipuFei in order to make some changes I pointed out (fixes to docs were merged yesterday).

[uranusjr]

You need a from __future__ import annotation line in the file to make CI work. (Edit: And a license blob. See contribution guide for help setting up pre-commit to check that locally.)

[LipuFei]

I have done a rebase. Hope it works. :) Thanks @potiuk

[LipuFei]

You need a from __future__ import annotation line in the file to make CI work. (Edit: And a license blob. See contribution guide for help setting up pre-commit to check that locally.)

Done:

• Added the LICENSE blob
• Added from __future__ import annotation

[LipuFei]

One thing to add though: https://github.com/apache/airflow/blob/main/docs/conf.py#L248 should also include taskinstancekey and it should also be added in https://github.com/apache/airflow/blob/main/docs/apache-airflow/public-airflow-interface.rst nest to taskinstance package as they are part of the public Airflow interface.

I just added taskinstancekey to docs that you have mentioned @potiuk :

• https://github.com/apache/airflow/blob/main/docs/conf.py#L248
• https://github.com/apache/airflow/blob/main/docs/apache-airflow/public-airflow-interface.rst

[o-nikolas]

Note: This will conflict with some changes I have had in PR for a couple weeks now #30727, last to merge will have to resolve it

[potiuk]

Note: This will conflict with some changes I have had in PR for a couple weeks now #30727, last to merge will have to resolve it

BTW. One of my first responses to this one (I did not send it) started with:

This looks cool, but @o-nikolas might get a bit angry if we merge this one before #30727 is merged.

I am looking at #30727 now.

[potiuk]

General comment here.

Honestly I think we should merge that one first - because it is very likely we are going to cherry-pick this one to 2.6.1 while #30727 is much more risky to cherry-pick to bugfix branch. This one should cherry-pick cleanly if we merge it first, whereas if we merge it after #30727 it will require manually applying the same fix to 2.6.1 branch.

This is also why I hesitated a bit with #30727 refactoring and where my comment about possibly splitting it even further came from @o-nikolas. This is a nice example how such refactorings are impacting our bug-fixing releases.

Something to consider when we decide to merge #30727 (also something for @ephraimbuddy and @pierrejeambrun to take a look I guess) - for me the kind of more complex refactors in "core" parts of Airflow are better done closer to the end of cherry-picking cycle when we are preparing to cut-off a new minor branch (this is one of the reasons why the base_job refactor of mine was done when it was done - after the last cherry-pickign round for 2.5.*)

[boring-cyborg]

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.