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

[DA][testing] RFC: Create AutomationConditionTester class #22292

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

This is an initial forray into exposing a testing API for AutomationConditions. It's quite minimal at the moment, but contains the necessary functions for a user to test how either new materializations or the passage of time might impact the decisions made. It's inspired by the testing framework we use internally, but with a bit less magic.

One constraint here was that any return value from this must only use public APIs, meaning no AssetKeyPartitionKey or AssetSubset, hence the somewhat weird shape of the AutomationConditionTesterResult object, and the fact that we just log the evaluation results rather than return them outright.

This contains a single test at the moment, roughly indicating how a user might interface with this, but just wanted to put this out to assess the vibes on this type of pattern.

How I Tested These Changes

Copy link
Contributor Author

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

Join @OwenKephart and the rest of your teammates on Graphite Graphite

@OwenKephart OwenKephart force-pushed the 06-04-Create_AutomationConditionTester branch from d217f3f to 8aba3a2 Compare June 4, 2024 22:39
@schrockn
Copy link
Member

schrockn commented Jun 6, 2024

I think this is too complicated for a public API. The primary interface should be a simple function with this as an opt-in advanced API.

@OwenKephart
Copy link
Contributor Author

@schrockn hm so the alternative testing path with a single function would look like:

from dagster import execute_automation_tick, AutomationTickResult

def my_test():

    instance=DagsterInstance.ephemeral()

    result = execute_automation_tick(defs=defs, current_time=..., instance=instance)

    assert result.num_requested == 1

    result = execute_automation_tick(defs=defs, current_time=..., cursor=result.cursor, instance=instance)

    assert result.num_requested == 0

This isn't the end of the world but is a bit more verbose because you have to explicitly shuttle the instance and cursor between ticks.

The main thing I was trying to accomplish with the class was to avoid that, but there's definitely no harm in starting lightweight.

@OwenKephart OwenKephart force-pushed the 06-04-Create_AutomationConditionTester branch from 8aba3a2 to 18549a4 Compare June 6, 2024 23:10
@OwenKephart
Copy link
Contributor Author

@schrockn updated the PR to move the class thing out of the public API and add an evaluate_automation_conditions method.

@OwenKephart OwenKephart marked this pull request as ready for review June 6, 2024 23:11
@OwenKephart OwenKephart requested review from schrockn and removed request for schrockn June 6, 2024 23:11
@schrockn
Copy link
Member

schrockn commented Jun 7, 2024

Yeah I think the function version is simpler and easier to understand. If we want to handle instance and context management I think we could do something like

with automation_testing(context=context, instance=instance):
    result = execute_automation_tick(defs=defs, current_time=...)

or

with automation_state(context=context, instance=instance, current_time=...) as state:
    result = state.execute_automation_tick(defs=defs)

and it just is a partial underneath the hood rather than a stateful builder thing.

Comment on lines 55 to 58
def evaluate_automation_conditions(
defs: Definitions,
instance: DagsterInstance,
asset_selection: AssetSelection = AssetSelection.all(),
evaluation_time: Optional[datetime.datetime] = None,
cursor: Optional[AssetDaemonCursor] = None,
logger: Optional[logging.Logger] = None,
) -> EvaluateAutomationConditionsResult:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should figure out a way to allow for passing a list of AssetsDefinitions

@OwenKephart OwenKephart force-pushed the 06-04-Create_AutomationConditionTester branch from 18549a4 to 59f0cae Compare June 7, 2024 16:56
@OwenKephart OwenKephart requested a review from schrockn June 7, 2024 16:56
@OwenKephart OwenKephart force-pushed the 06-04-Create_AutomationConditionTester branch from 59f0cae to 234edf7 Compare June 7, 2024 16:58
@OwenKephart
Copy link
Contributor Author

@schrockn gotcha -- removed the class-based thing, made defs accept a Sequence[AssetsDefinition], and added a docstring

respect_materialization_data_versions=False,
auto_materialize_run_tags={},
)
results, requested_asset_partitions = evaluator.evaluate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely should be a goal to have this return a range representation soon. cc: @smackesey

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool thanks

@OwenKephart OwenKephart merged commit a7bdb7a into master Jun 7, 2024
1 check failed
@OwenKephart OwenKephart deleted the 06-04-Create_AutomationConditionTester branch June 7, 2024 19:19
danielgafni pushed a commit to danielgafni/dagster that referenced this pull request Jun 18, 2024
…#22292)

## Summary & Motivation

Adds a user-facing method for evaluating asset conditions

## How I Tested These Changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants