Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
Avihais12344 opened this issue Aug 13, 2024 · 6 comments
Closed
2 tasks done

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

Avihais12344 opened this issue Aug 13, 2024 · 6 comments
Labels
area:core kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet pending-response stale Stale PRs per the .github/workflows/stale.yml policy file

Comments

@Avihais12344
Copy link

Avihais12344 commented Aug 13, 2024

Description

Instead of using list for holding the tags of a specific dag (as we can see at the docs), I suggest using set instead, specifically MutableSet for the type hinting, and python set as the default implementation.

Use case/motivation

I would want to add multiple tags (even the same ones) faster, without worrying about duplicates, and reduce the memory usage because of it. Also, it's good to check if a certain tag already exists.

Related issues

No response

Are you willing to submit a PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@Avihais12344 Avihais12344 added kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet labels Aug 13, 2024
Copy link

boring-cyborg bot commented Aug 13, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@dosubot dosubot bot added the area:core label Aug 13, 2024
@sunank200
Copy link
Collaborator

@Avihais12344 Thanks for raising the issue. I had follow-up questions:

  • Will this change to set break any existing DAGs or workflows expecting a list? I think it would. Wouldn't this be breaking change?
  • Are there any edge cases or performance regressions to consider with this change?
  • How would we ensure the use of set is consistent across the codebase, particularly in how tags are stored/retrieved?

@Avihais12344
Copy link
Author

Avihais12344 commented Aug 21, 2024

Hello @sunank200, thanks for responding.

It would break existing code expecting tags as list, that's why I wanted to discuss it here.
The interface is different from list, but other than that, it would not be too much than different, we can think of it as a unique list. So I think there would not be edge case, as we alreay take the unique items from the tags list.

We can ensure the use of set accross the codebase by using typing and linters that would enforce the right types. I would also use the collections.abc type of MutableSet instead of the builtin set, to allow any type of set.

@Avihais12344
Copy link
Author

Created a PR for this here.

Copy link

github-actions bot commented Sep 7, 2024

This issue has been automatically marked as stale because it has been open for 14 days with no response from the author. It will be closed in next 7 days if no further activity occurs from the issue author.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 7, 2024
Copy link

This issue has been closed because it has not received response from the issue author.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet pending-response stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

No branches or pull requests

2 participants