-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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 dags' tags #41695
base: main
Are you sure you want to change the base?
Changes from 6 commits
3881820
079ebb2
cfaa915
c4534de
a03c055
b31dcca
bd24f30
a20eeb7
c5c2e02
49c7743
8a611d2
9432103
659d75b
816d73e
1e8af76
ce824cb
40605cd
557ced7
41e5112
035e50a
6a433ef
669ede1
fd652b3
08a5689
eb20c7b
d43f204
64dcb34
bae6471
18773a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Replaced Python's `list` with `collections.abc.MutableSet` for the `Dag` tags. | ||
romsharon98 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1060,7 +1060,7 @@ def test_bulk_write_to_db(self): | |
|
||
# Removing all tags | ||
for dag in dags: | ||
dag.tags = None | ||
dag.tags = set() | ||
with assert_queries_count(9): | ||
DAG.bulk_write_to_db(dags) | ||
with create_session() as session: | ||
|
@@ -3720,6 +3720,30 @@ def test__tags_length(tags: list[str], should_pass: bool): | |
DAG("test-dag", schedule=None, tags=tags) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"input_tags, expected_result", | ||
[ | ||
pytest.param([], set(), id="empty tags"), | ||
pytest.param(["a normal tag"], {"a normal tag",}, id="one tag"), | ||
pytest.param(["a normal tag", "another normal tag"], {"a normal tag", "another normal tag"}, id="two different tags"), | ||
pytest.param(["a", "a"], {"a",}, id="two same tags"), | ||
], | ||
) | ||
def test__tags_duplicates(input_tags: list[str], expected_result: set[str]): | ||
result = DAG("test-dag", tags=input_tags) | ||
assert result.tags == expected_result | ||
|
||
|
||
def test__tags_mutable(): | ||
expected_tags = {"6", "7"} | ||
test_dag = DAG("test-dag") | ||
test_dag.tags.add("6") | ||
test_dag.tags.add("7") | ||
test_dag.tags.add("8") | ||
test_dag.tags.remove("8") | ||
assert test_dag.tags == expected_tags | ||
Comment on lines
+3412
to
+3419
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to support this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall I want to check behaviour, if we change the behaviour, we should know about it. That's why I think we should have this test, but let's keep the conversation here, as we talk about the same thing. |
||
|
||
|
||
@pytest.mark.need_serialized_dag | ||
def test_get_dataset_triggered_next_run_info(dag_maker, clear_datasets): | ||
dataset1 = Dataset(uri="ds1") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably just use
Collection
. We don’t really expect users to add more tags to a DAG after it’s created.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's true. In cluster policies, we may want to add custome tags to DAGs (for orginization hierarcy for example).
And I don't think it would be correct to make it an immutable collection that we would need to create every time we want to change something.
If we would go with
Collection
, we would go with a different programing paradigram, and I am not sure everyone would like it, so I would keep the door open and let everyone chose what they want to.