Propagate automation condition information through the UI for user code conditions

[OwenKephart]

Summary & Motivation

It is not possible to serialize the full automation condition object if it is user-defined. In these cases, we only persist the snapshot.

Updates the graphql code to use this snapshot instead of the raw condition, and adds e2e test ensuring we can execute / get the correct label for these objects.

How I Tested These Changes

Changelog

NOCHANGELOG

[OwenKephart]

• [1.9] Update evaluation id logic to be in sync with tick ids #25269
• [1.9] Add automation_condition to observable_source_asset #25344
• [1.9] Create default_automation_condition_sensor in Definitions object #25201
• [1.9] Update deprecation warnings for automation_condition, auto_materialize_policy, and freshness_policy #25208
• [1.9] Remove experimental warnings from AutomationCondition static constructors #25207
• [1.9] Promote hidden params to top-level args in AutomationConditionSensorDefinition #25206
• [1.9] Update default value of allow_backfills to True #25204
• Add executed_with_root_target condition to handle partial runs / failures #25253
• Add error for user code sensors with too many entities targeted #25215
• Add test demonstrating that we do not hit the RESOURCE_EXHAUSTED error above a given threshold #25214
• Propagate automation condition information through the UI for user code conditions #25211 👈
• Export AutomationContext and AutomationResult #25210
• Store allow_backfills in the sensor metadata #25203
• Create backfill_in_progress AutomationCondition #25190
• [refactor] Move all operands into a single file #25187
• Remove description property, make names more descriptive #25186
• Simplify return types of static constructors for AutomationCondition #25184
• Add all_deps_blocking_checks_passed condition #25171
• Make existing conditions work with Asset Checks #25170
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @OwenKephart and the rest of your teammates on Graphite

[OwenKephart]

@gibsondan flagging this PR as at one point we removed this data from the snapshot due to (iirc?) unfounded beliefs that it was blowing up the workspace size -- just confirming that this was actually unfounded

[gibsondan]

I think it was in fact visibly increasing the workspace size but it remains unclear what the exact impact of that was - worth a quick chat maybe

…

On Thu, Oct 10, 2024 at 7:41 PM OwenKephart ***@***.***> wrote: ***@***.**** commented on this pull request. @gibsondan <https://github.com/gibsondan> flagging this PR as at one point we removed this data from the snapshot due to (iirc?) unfounded beliefs that it was blowing up the workspace size -- just confirming that this was actually unfounded — Reply to this email directly, view it on GitHub <#25211 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAPJCZP27KISBEL745WMADZ24NE7AVCNFSM6AAAAABPX3XBTCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNRRG4YDGNZSG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[gibsondan]

is there a world where we only persist this new thing once you opt in to user-defined automation condition sensors? or does that make things way more complicated?

[gibsondan]

sounds like that world is indeed possible

updated

[schrockn]

[Re: lines +336 to +349]

can we not just _always_ use the snapshot?

See this comment inline on Graphite.

[OwenKephart]

Ah we need to use the AutomationConditionEvaluation in the history UI, as we don't store the full snapshot there. We could put a method on AutomationConditionEvaluation that reconstructs an AutomationConditionSnapshot though, just seems out of scope for this PR. Can stack one a PR for that on top though

[schrockn]

Why don't we just always use the snapshots?

[OwenKephart]

In general, because we need to be able to execute the conditions on the daemon side of the process if executing in daemon mode. In the code sample you linked to, no good reason (and I removed it).

updated

[schrockn]

Don't see why we don't unconditionally use the snapshot in the graphql server

[schrockn]

As in why can't the asset_node_snap just have the snapshot?

[OwenKephart]

Responded to this above

[schrockn]

This is just a weird mental model in my mind.

Seems like there should be just a snapshot but we should have special logic to reconstruct the "real" automation condition in the asset daemon from the snapshot.

[bengotow]

Just reviewed the UI + graphql portions of this but it looks good to me! Left one inline comment, might be nice to remove the autoMaterializePolicy fields from the graphql queries if we can.

[OwenKephart]

@schrockn The snapshot is designed to only contain information that's relevant to displaying a condition in the UI to a user. This means that it reduces down a lot of the complexity of the parameters etc. of a given condition to being just a simple string (e.g. the any_deps_match condition can have an "allow" or "ignore" parameter, which are rendered in the string like so: https://sourcegraph.com/github.com/dagster-io/dagster@a118eeb03d2c9a6f2a8ff1c152f4a0acc52e4a7a/-/blob/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/dep_operators.py?L76

In general, this means that we don't enforce any contract on how those names should be generated, and so trying to deserialize them is impossible, and much more the job of the serdes framework.

So if we wanted to avoid having both an AutomationCondition field and an AutomationConditionSnapshot field on the asset_node_snap, then we'd need to go the route of only populating the AutomationCondition field, which would necessitate having a special "NonExecutableAutomationCondition" or something that we could transform user-defined conditions into, whose purpose would be to allow us to get the parts of user-defined automation conditions that are necessary to render it in the UI across the serdes boundary. I explored this approach in a previous PR but we deemed that to be a confusing system, so that approach was abandoned, but I could be convinced to go back to that setup.

responded