Forgetting asset checks on Definitions

[johannkm]

A theme across internal and now external dogfooding is that it's easy to forget to include checks in the Definitions object.

• you have to remember for each individual @asset_check. We don't have load_asset_checks_from_*methods
• checks defined with @asset(check_specs=[...]) are included along with the asset. Confusing difference vs @asset_check
• currently if you dagster dev -f repo.py and don't define a Definitions we grap all asset defs but not checks

This gives a bad first impression of checks, so we should mitigate it. Our options:

1. Documentation (definitely)

We need to emphasize Definitions(asset_checks=[...]) in our docs.

2. Load asset checks by default (definitely)

Load checks along with assets when you call dagster dev -f repo.py without a defs = Definitions(...).

3. load_asset_checks_from_*

We could offer a compliment to load_assets_from_*. My concerns here are that you still have to remember to do

defs = Definitions(assets=load_assets_from_current_module(), asset_checks=load_asset_checks_from_current_module())

and you likely always will do a matching load_assets... and load_asset_checks.... Still, I think this is an improvement versus the current

from .path.to.asset_a import check_a
from .path.to.asset_b import check_b

defs = Definitions(assets=load_assets_from_current_module(), asset_checks=[check_a, check_b])

4. load_defs_from_* (planned post 1.5)

We've discussed a generic function for loading all definitions from a module.

defs = load_defs_from_current_module()

# maybe...
defs = load_defs_from_current_module() + load_defs_from_package_name("foo")

This handles the concerns about duplicate load_assets... and load_asset_checks.... It's also a larger change that we won't be able to make for 1.5 this week. Users on existing load_assets_... methods would have to migrate.

5. Reference @asset_checks on @asset

Change the APIs to something to the tune of

@asset_check(asset_key='my_asset')
def my_check():
  ...

@asset(checks=[my_check, AssetCheckSpec('my_other_check', asset='my_asset')])
  yield AssetCheckResult(success=True, check_name='my_other_check')

and include the checks along with the asset.

• now checks have to be defined before the asset
• check defs still need a target asset in their definition. If we take away that constraint, we get in to the world of dagster parameterized checks that @sryza breifly explored and decided against, in favor of check factories
• we'd no longer need Definitions(asset_checks=[...]), but could leave it for rare use cases where users can't pass the check in to the asset

[johannkm]

@schrockn has suggested load_defs_from_* as a followup to 1.5. We need to decide if it will be an adequate solution, and if so, are we ok shipping without it/do we need to do something else before 1.5.x when we add support

[sryza]

1. Documentation (definitely)

+1

2. Load asset checks by default (definitely)

+1

3. load_asset_checks_from_*

I favor this direction for the near term. As a helper function, I think the cost of offering this is pretty low, and I think it will be the easiest thing to adopt incrementally if you're already using load_assets_from_*.

4. load_defs_from_* (planned post 1.5)

I'm optimistic about this direction, but I think it will take a little while to get there, and I'm not convinced that it will be the right API in all cases. Users in the existing paradigm might have structured things in a way that makes this difficult to take advantage of, and there's value in offering a more incremental path to bulk loading asset checks.

5. Reference @asset_checks on @asset

We considered this in the early design phase of asset checks and decided against it. Here are some of the reasons:

• What about a single AssetChecksDefinition that checks multiple assets? Would it need to be included on every asset that's targeted by a check inside it?
• You end up needing to double-specify the asset that a check targets, both on the check definition and by attaching the check to the asset. Alternatively, checks become generic by default - unbound to any particular asset. However, this adds complexity to our ontology that I think it would be better to be handled in user space. It introduces a layer of indirection, and inevitably begs questions about how you parameterize these generic checks.
• It's easier to get confused about whether the check is executing in a separate op or the same op as the asset it's included on.
• If you want to have a DSL where you specify checks using config and then use a factory to generate a bunch of check definitions from that config, you end up needing to zip up those checks with your assets definitions.

[schrockn]

Yeah re: 5 I'm not actively pushing for it, but I suggested Johann write out the option for purposes of rigor and completeness.

The fastest way to implement this would be to change @asset_check to return a different object type ('UnboundAssetChecksDefinitionor some less bad name) than the currentAssetChecksDefinitionand then change thecheck_specsargument tochecks` and have it take that new object, potentially.

What about a single AssetChecksDefinition that checks multiple assets? Would it need to be included on every asset that's targeted by a check inside it?

There would need to be a separate notion of a "standalone" asset check to cover this case, and cases like an asset check in a separate code location (which I think is a perfectly valid use case).

You end up needing to double-specify the asset that a check targets, both on the check definition and by attaching the check to the asset. Alternatively, checks become generic by default - unbound to any particular asset. However, this adds complexity to our ontology that I think it would be better to be handled in user space. It introduces a layer of indirection, and inevitably begs questions about how you parameterize these generic checks.

This wouldn't be true if @asset_check returned a different object designed to be fed into AssetsDefinition.

It's easier to get confused about whether the check is executing in a separate op or the same op as the asset it's included on.

To me this is the biggest downside and perhaps disqualifying.

If you want to have a DSL where you specify checks using config and then use a factory to generate a bunch of check definitions from that config, you end up needing to zip up those checks with your assets definitions.

There would still be a standalone AssetChecksDefinition class that would definitely be used in the factory case.

[schrockn]

But yes I am bullish on load_defs_from_module approach to solve this as well.

[askvinni]

There would need to be a separate notion of a "standalone" asset check to cover this case, and cases like an asset check in a separate code location (which I think is a perfectly valid use case).

This is actually something that I personally would love to see. The way we're setting up Dagster at Staffbase is by making heavy use of a utils lib that contains all sorts of helper functionality. Currently it holds a lot of asset check factories.

In the short/mid-term we're working on a separate code location that's responsible for monitoring asset health and maintaining assets with data quality metrics, so moving some checks over there would make a lot of sense.

[johannkm]

Because none of us are very excited for #5, let's not do it for 1.5. We'll add load_asset_checks_from_* and followup with load_defs_from_*.