search

dagster-io / dagster

An orchestration platform for the development, production, and observation of data assets.
https://dagster.io
Apache License 2.0
11.91k stars 1.49k forks source link

Do not collapse `AutomationCondition`s with labels #26097

Closed deepyaman closed 3 days ago

deepyaman commented 4 days ago

Summary & Motivation

Imagine you have:

a = (AutomationCondition.foo() & AutomationCondition.bar()).with_label("something")
b = a & AutomationCondition.baz()

The implementation of __and__ automatically transforms And(And(c1, c2), c3) into And(c1, c2, c3). This process is generally desirable, as this reduces the number of levels in the hierarchy, making it easier to parse and understand expressions.

However, if an expression has a label, then this indicates that it has its own semantic meaning, which gets erased by this process.

We should prevent this automatic collapsing in cases where the existing condition's label is not None.

The same applies to __or__!

How I Tested These Changes

Added unit tests in python_modules/dagster/dagster_tests/definitions_tests/declarative_automation_tests/automation_condition_tests/fundamentals/test_automation_condition.py.

Changelog

Fixed a bug where using the & or | operators on AutomationConditions with labels would cause that label to be erased.

OwenKephart commented 3 days ago

Also let's update the Changelog entry to:

Fixed a bug where using the & or | operators on AutomationConditions with labels would cause that label to be erased.

before merging

deepyaman commented 3 days ago

This looks great! Not a blocker, but just because it's only a few extra lines of code, might be worth adding a test for both the first and second operands being labeled

Sounds good, will do!