Use set instead of list for dags' tags #41695

Avihais12344 · 2024-08-23T12:21:09Z

Closes issue 41420.

Thank you for viewing this PR :)

^ 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.

…o check duplications.

boring-cyborg · 2024-08-23T12:21:18Z

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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.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.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
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

shahar1

Overall, I agree with the idea that set is more appropriate in this case, as tags should be unique. I'd be happy for more opinions whether it's worth making it as a breaking chage.

airflow/models/dag.py

tests/models/test_dag.py

newsfragments/41420.significant.rst

…3.8.

jscheffl

I like this cleanup! Especially as is is "non-breaking" for existing DAGs, so no effect on DAG authors but better data types. Just small proposals to pin the typing to strings

airflow/models/dag.py

jscheffl

Some tests fail, otherwise OK for me. Small nit as comments

newsfragments/41420.significant.rst

airflow/models/dag.py

Co-authored-by: Jens Scheffler <[email protected]>

uranusjr · 2024-09-11T20:08:54Z

I do not like this, but feel free to merge this without me if others feel more strongly this i beneficial.

…it raises the error: Subscripted generics cannot be used with class and instance checks.

Avihais12344 · 2024-09-12T06:03:10Z

@uranusjr why you don't like this?

uranusjr · 2024-09-12T17:37:47Z

I mostly outlined the reasons above, but to summarise, you want to keep this mutable so you can call add after a DAG is created, but by changing this from a list to set, you are also breaking everyone that’s already adding tags after a DAG is created. The two things you are trying to do (use set instead of list, and keep the attribute mutable) have directly conflicted reasoning. IMO it is simply not worthwhile to break everyone so you can more easily handle duplicated tags. You can just do that easily in your own code.

With that said, 3.0 is the exact time to break everyone, so I am still open for this to be merged if people feel fine about it. But I will not approve nor merge this change myself.

jscheffl · 2024-09-12T17:40:20Z

I mostly outlined the reasons above, but to summarise, you want to keep this mutable so you can call add after a DAG is created, but by changing this from a list to set, you are also breaking everyone that’s already adding tags after a DAG is created. The two things you are trying to do (use set instead of list, and keep the attribute mutable) have directly conflicted reasoning. IMO it is simply not worthwhile to break everyone so you can more easily handle duplicated tags. You can just do that easily in your own code.

With that said, 3.0 is the exact time to break everyone, so I am still open for this to be merged if people feel fine about it. But I will not approve nor merge this change myself.

Would it make you more happy if we keep the interface as a list (so: non breaking) but during setting/init it is temporarily converted to a set to ensure no duplicates are in the list?

Avihais12344 · 2024-09-15T06:41:18Z

@uranusjr I am sorry to hear you don't like my change. I think the solution of @jscheffl is reasonable to handle your conflict, even though I think we should break the interface to a set. Due to the release of Airflow 3, it's a great time (that may not come back) to break interfaces.

But can you (@uranusjr) at least close your conversations? As it blockes the merge request (that was approved by other people).
Thank you for your understanding.

boring-cyborg · 2024-09-21T09:30:44Z

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

* Started working on dag tags, moved the tags to set, and added atest to check duplications. * Fixed more areas of the c'tor. * Fixed test of dag tags. * Added a check to see if the tags are mutable. * Added newsfragment. * Removed unecessary check. * Removed str specification at the type, for compatability with python 3.8. * Removed more type specification as part of compatability with python 3.8 * Fixed the newsfragment. * Added missing word. * Used `` for code segemnts at the rst file. * Reformatted the file. * Fixed wrong method for adding tag. * Added type hinting at the dag bag. * Deserialized the tags to set. * Adjusted the tests for the set type. * Added type hinting. * Sorting the tags by name. * Changed to typing. * Update newsfragments/41420.significant.rst Co-authored-by: Jens Scheffler <[email protected]> * Update newsfragments/41420.significant.rst Co-authored-by: Jens Scheffler <[email protected]> * Removed the generic specification at the dag args expected types, as it raises the error: Subscripted generics cannot be used with class and instance checks. * Added tags to the expected serialized DAG. * Added sorting the tags keys by the name key. * Fixed sorting tags by name to use `sorted` instead of `.sort` * Fixed tags comparesion, as it's now a set, and not a list. --------- Co-authored-by: Jens Scheffler <[email protected]>

Avihais12344 added 4 commits August 23, 2024 13:19

Started working on dag tags, moved the tags to set, and added atest t…

3881820

…o check duplications.

Fixed more areas of the c'tor.

079ebb2

Fixed test of dag tags.

cfaa915

Added a check to see if the tags are mutable.

c4534de

Avihais12344 requested review from kaxil, XD-DENG and ashb as code owners August 23, 2024 12:21

Avihais12344 mentioned this pull request Aug 23, 2024

Use set instead of list for dag's tags #41420

Closed

2 tasks

shahar1 added airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes airflow3.0:candidate Potential candidates for Airflow 3.0 area:core labels Aug 23, 2024

shahar1 added this to the Airflow 3.0.0 milestone Aug 23, 2024

shahar1 reviewed Aug 24, 2024

View reviewed changes

Added newsfragment.

a03c055

uranusjr reviewed Aug 26, 2024

View reviewed changes

airflow/models/dag.py Outdated Show resolved Hide resolved

uranusjr reviewed Aug 26, 2024

View reviewed changes

airflow/models/dag.py Outdated Show resolved Hide resolved

uranusjr reviewed Aug 26, 2024

View reviewed changes

tests/models/test_dag.py Show resolved Hide resolved

Removed unecessary check.

b31dcca

romsharon98 reviewed Sep 2, 2024

View reviewed changes

newsfragments/41420.significant.rst Outdated Show resolved Hide resolved

Avihais12344 added 5 commits September 2, 2024 19:22

Merge remote-tracking branch 'upstream/main'

bd24f30

Removed str specification at the type, for compatability with python …

a20eeb7

…3.8.

Removed more type specification as part of compatability with python 3.8

c5c2e02

Fixed the newsfragment.

49c7743

Added missing word.

8a611d2

romsharon98 approved these changes Sep 4, 2024

View reviewed changes

jscheffl approved these changes Sep 4, 2024

View reviewed changes

airflow/models/dag.py Outdated Show resolved Hide resolved

airflow/models/dag.py Outdated Show resolved Hide resolved

airflow/models/dag.py Outdated Show resolved Hide resolved

Used `` for code segemnts at the rst file.

9432103

Avihais12344 requested review from uranusjr and shahar1 September 6, 2024 09:15

Avihais12344 added 6 commits September 6, 2024 16:34

Added type hinting at the dag bag.

1e8af76

Deserialized the tags to set.

ce824cb

Adjusted the tests for the set type.

40605cd

Added type hinting.

557ced7

Sorting the tags by name.

41e5112

Merge branch 'main' of github.com:Avihais12344/airflow

035e50a

Avihais12344 requested a review from bolkedebruin as a code owner September 6, 2024 14:05

Changed to typing.

6a433ef

Avihais12344 requested review from romsharon98 and jscheffl September 6, 2024 14:15

jscheffl approved these changes Sep 7, 2024

View reviewed changes

newsfragments/41420.significant.rst Outdated Show resolved Hide resolved

newsfragments/41420.significant.rst Outdated Show resolved Hide resolved

airflow/models/dag.py Outdated Show resolved Hide resolved

Avihais12344 and others added 2 commits September 8, 2024 19:47

Update newsfragments/41420.significant.rst

669ede1

Co-authored-by: Jens Scheffler <[email protected]>

Update newsfragments/41420.significant.rst

fd652b3

Co-authored-by: Jens Scheffler <[email protected]>

Avihais12344 requested a review from jscheffl September 8, 2024 16:47

jscheffl approved these changes Sep 8, 2024

View reviewed changes

Removed the generic specification at the dag args expected types, as …

08a5689

…it raises the error: Subscripted generics cannot be used with class and instance checks.

Avihais12344 requested a review from jscheffl September 12, 2024 06:00

Avihais12344 added 5 commits September 20, 2024 15:18

Added tags to the expected serialized DAG.

eb20c7b

Added sorting the tags keys by the name key.

d43f204

Merge remote-tracking branch 'upstream/main'

64dcb34

Fixed sorting tags by name to use sorted instead of .sort

bae6471

Fixed tags comparesion, as it's now a set, and not a list.

18773a7

romsharon98 merged commit bdab7dc into apache:main Sep 21, 2024
51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use set instead of list for dags' tags #41695

Use set instead of list for dags' tags #41695

Avihais12344 commented Aug 23, 2024 •

edited

Loading

boring-cyborg bot commented Aug 23, 2024

shahar1 left a comment

jscheffl left a comment

jscheffl left a comment

uranusjr commented Sep 11, 2024 •

edited

Loading

Avihais12344 commented Sep 12, 2024 •

edited

Loading

uranusjr commented Sep 12, 2024

jscheffl commented Sep 12, 2024

Avihais12344 commented Sep 15, 2024

boring-cyborg bot commented Sep 21, 2024

Use set instead of list for dags' tags #41695

Use set instead of list for dags' tags #41695

Conversation

Avihais12344 commented Aug 23, 2024 • edited Loading

boring-cyborg bot commented Aug 23, 2024

shahar1 left a comment

Choose a reason for hiding this comment

jscheffl left a comment

Choose a reason for hiding this comment

jscheffl left a comment

Choose a reason for hiding this comment

uranusjr commented Sep 11, 2024 • edited Loading

Avihais12344 commented Sep 12, 2024 • edited Loading

uranusjr commented Sep 12, 2024

jscheffl commented Sep 12, 2024

Avihais12344 commented Sep 15, 2024

boring-cyborg bot commented Sep 21, 2024

Avihais12344 commented Aug 23, 2024 •

edited

Loading

uranusjr commented Sep 11, 2024 •

edited

Loading

Avihais12344 commented Sep 12, 2024 •

edited

Loading