@VirginiaPoundstone, @xcollazo , @mforns : do you think it would be possible to tackle this?

If an incoming Airflow DAGs revision changes a DAG's DagProperties, then the corresponding Airflow variable should also be updated to reflect the change.

This was surprising to me as well when I started using DagProperties, but it now makes sense, and recently while developing a similar Airflow Pool definition mechanism, I also adopted the same semantics: if there is an inconsistency between the YAML files and the Airflow DB, the DB wins.

I think we want this behavior because the whole reason we develop these mechanisms is for folks to be able to override them via the Airflow UI, instead of having to go thru the whole deployment process. In that case, you want those overrides to win, because otherwise the overrides would not kick in.

Example: You set your conda artifact to artifact=v1.conda. Then you develop a new version, but you do not want to go over the deployment process. So you go to Airflow UI and set that DagProperties to artifact=v2.conda. Later, you change something about the DAG unrelated to the conda artifact and you forget to also update the conda setting that you had overridden manually. In that case, we want artifact=v2.conda to win, which is not reflected on your DAG but on the Airflow DB.

If you want the DAG to always win, then we have to manually delete the DagProperties definition in the Airflow UI. I know this is not how Structured Data typically deploys, but this pattern is how most folks typically use these DagProperties for. We could brainstorm if we could somehow automate the deletion of the DagProperties definition as part of your deployment process.

CC @mforns so that we are in sync.

if the incoming DagProperties no longer has a given property, then the DAG can't be parsed because the given property can't be overridden anymore

Right, I have seem this one and agree it is annoying. We could modify this behavior to just ignore it. But: what if you just mispelled a DagProperties? If we ignore it, then you would never know.

WDYT?

Agree with both!

It is annoying that when you intentionally modify a property in your code, it is immediately overridden by the old Airflow variable. And also that when you remove a property in your code, the DAG parsing breaks when trying to compute the old override.

At the same time, as Xabriel mentions, the property values in the Airflow variable must always win (override) by definition and that's the main purpose of those. The fact that when you deploy a DAG for the first time, the Variable is created by default and populated with all original values was also intentional, to reduce the work of having to create the Airflow variable manually (writing the JSON blob), which was a pain of the previous version of this lib (VariableProperties). This however, makes it so that whenever you remove a property from the code, the DAG parsing breaks until you remove the property from the Airflow variable too. And I think this is good, since we want to fail fast, and raise potential errors, instead of letting them pass.

That said, I'd love to make the DagProperties lib more magical, and add tricks to mitigate the caveats pointed out! Some ideas:

• Adding a record to the Airflow variable that stores a short checksum of each property in the JSON blob.
• When the DAG processes the JSON blob, it will know whether a property override is new (different from its checksum).
• In this case, the DAG will override the property value, and update the checksum in the Airflow variable.
• If there's a difference between a code property and its variable override, but the checksum still checks (variable property has not changed), it means the code change should prevail,
• In this case, the DAG will keep the code value and update the Airflow variable accordingly.
• This last use case, could take care of missing properties as well I think.

I think the solution I propose above does not work, I think this assumption is wrong:

If there's a difference between a code property and its variable override, but the checksum still checks (variable property has not changed), it means the code change should prevail,

If we did this, any override would change the DAG value just for one parsing cycle. The second parsing of the DAG would reset the property value to the default.

I think the checksum might need to contain the state of both the default values coming from the code AND the override values coming from the Airflow Variable.

Maybe, for each property, we could have two 3-letter hashes (i.e. "X3p", "Ql1") that represent the value of the default value and the override value respectively.
We could store these all together in the same string in alphabetical order of the property names, like:

"<p1_default_hash><p1_override_hash>-<p2_default_hash><p2_override_hash>-..."

It would look something like:

"Hw3rTa-Y45Mmb-..."

When parsing the DAG, the DagProperties module can check where the change comes from by looking at which checksum doesn't match the value.

• If the change comes from the Airflow variable (override), DagProperties should override the value, calculate the new checksums and update the variable with them.
• If the change comes from the code (default), DagProperties should set the property to its new default, calculate the new checksums and update the variable with both the new value and checksum.
• If there's no change, we're good.

We could add the checksum as a property of the Variable, and name it something that reminds the developer to not edit it. Like:

"__checksum_do_not_edit__": "Hw3rTa-Y45Mmb-..."

But even if it's edited and we lose the state, DagProperties can assume the Variable property values are the correct ones and override, which is what DagProperties is doing nowadays.
It would do it just if the checksum is corrupted (edited by mistake).

Alternative idea:

When DagProperties populates the Airflow Variable, make it fill in the property values with some dummy "__DEFAULT__" value it understands. If a value is "__DEFAULT__", then the Dag will use whatever is in DagProperties in the dag code itself. If a user wants to override a specific value, they can edit the Variable and change "__DEFAULT__" to whatever they want.

This way, a value update in dag code will be applied unless the user has explicitly gone and and overridden the Variable's property value themself.

This doesn't solve the issue of modified property keys (new or removed), but at least it would work they way I'd expect: use code value unless there is an explicit override in Airflow Variable.

if the incoming DagProperties no longer has a given property, then the DAG can't be parsed because the given property can't be overridden anymore

Right, I have seem this one and agree it is annoying. We could modify this behavior to just ignore it. But: what if you just mispelled a DagProperties? If we ignore it, then you would never know.

Ignoring seems better than the current situation to me.

the Variable is created by default and populated with all original values was also intentional, to reduce the work of having to create the Airflow variable manually (writing the JSON blob), which was a pain of the previous version of this lib

Or maybe, we should just not do this. I don't think the pain of having to write the JSON blob is worse than the pain of the unexpected behavior during deployments.

We could also restrict the creation of Airflow Variables and all the overriding to the dev environment.
And let the DagProperties module be a no-op in production.

mforns opened https://gitlab.wikimedia.org/repos/data-engineering/airflow-dags/-/merge_requests/2049

Disable DagProperties overrides in production