[dagster-dbt] Support asset checks in dbt_cloud_assets

[maximearmstrong]

Summary & Motivation

This PR updates the dbt_cloud_assets decorator to pass the check specs to the multi-asset definition, and updates DbtCloudJobRunResults.to_default_asset_events to yield AssetCheckResults when asset check are executed in an asset execution context.

How I Tested These Changes

Updated test with BK

[maximearmstrong]

• [dagster-dbt] Support asset checks in dbt_cloud_assets #28907 👈 (View in Graphite)
• [dagster-dbt] Support dbt selections for dbt Cloud #28906
• [dagster-dbt] Support context in default asset events for dbt Cloud #28905
• [dagster-dbt] Update args with context in DbtCloudWorkspace.cli(..) #28902
• [dagster-dbt] Update DbtCloudCliInvocation to accept manifest #28901
• [dagster-dbt] Implement basic dbt_cloud_assets decorator #28886
• [dagster-dbt] Implement DbtCloudCliInvocation #28152
• [5/n][dagster-dbt] Implement load_dbt_cloud_asset_specs and load_dbt_cloud_check_specs #28151
• [4/n][dagster-dbt] Implement fetch_workspace_data for DbtCloudWorkspace #28150
• [3/n][dagster-dbt] Add artifact methods in DbtCloudJobRunHandler #28149
• [2/n][dagster-dbt] Add DbtCloudJobRunHandler #28148
• [1/n][dagster-dbt] Add DbtCloudWorkspace and DbtCloudWorkspaceClient for dbt Cloud v2 #28147
• master

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

[graphite-app]

There's an issue with the severity assignment for passing tests in AssetCheckResult. Currently, when a test passes (result_status == TestStatus.Pass), the severity is still set to AssetCheckSeverity.ERROR because of how the conditional is structured.

According to the AssetCheckSeverity documentation, ERROR should only indicate actual issues with the asset. For passing tests, the severity parameter should either be omitted or explicitly set to None.

Consider modifying the severity assignment to only apply for failing tests:

severity=(
    AssetCheckSeverity.WARN if result_status == TestStatus.Warn
    else AssetCheckSeverity.ERROR if result_status != TestStatus.Pass
    else None
)

This ensures that passing tests don't incorrectly receive an error severity level.

Suggested change

	yield AssetCheckResult(
	passed=result_status == TestStatus.Pass,
	asset_key=asset_check_key.asset_key,
	check_name=asset_check_key.name,
	metadata=metadata,
	severity=(
	AssetCheckSeverity.WARN
	if result_status == TestStatus.Warn
	else AssetCheckSeverity.ERROR
	),
	)
	yield AssetCheckResult(
	passed=result_status == TestStatus.Pass,
	asset_key=asset_check_key.asset_key,
	check_name=asset_check_key.name,
	metadata=metadata,
	severity=(
	AssetCheckSeverity.WARN if result_status == TestStatus.Warn
	else AssetCheckSeverity.ERROR if result_status != TestStatus.Pass
	else None
	),
	)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

[dpeng817]

I don't remember the exact semantics here but this looks potentially real. Worth looking into.

[dpeng817]

There's something weird here. If the asset check wasn't selected and we yield an evaluation, reasonably sure that we error in the system.
In general feels like there's some duplication from dagster dbt which is desirable but it's hard to know what is what bc the code is being duplicated rather than reused.

I understand that it's hard to directly reuse dbt behavior due to munging from a different artifact but I want to better understand in this case what's new vs what's coming from the old integration. And potentially create an adapter layer so that all this conversion happens in one place

[dpeng817]

And this means that it's an asset check in either direct invocation or that the check wasn't selected? Does this mirror DBT behavior?

[maximearmstrong]

I've double checked and I think the condition should be elif not has_asset_def and asset_check_key is not None instead. We should not yield an evaluation in an asset context if the check was not selected. This would also reflect the behavior in dbt core.

In dbt core, we use asset observations for tests that are executed but not selected. I agree that we should share the code between dbt core and dbt Cloud here to avoid conflicts and behavior discrepancy. I will update the condition for this PR, and reuse the same code for both integrations in another PR.

[dpeng817]

I'll approve with the assumption that the comments will be cleaned up and that the weirdness here regarding AssetCheckEvaluation is inherited from dbt core integration. We should definitely consolidate.

[dpeng817]

I still really don't understand this line of code. Like; why is it ok to yield an asset check evaluation in a non asset context? Is it because we don't coerce them?

[dpeng817]

Feels weird to just arbitrarily check metadata; we should be checking for the particular metadata we care about