[module-loaders] Further simplify asset checks path

[dpeng817]

Summary & Motivation

We can further simplify the load_asset_from_x code path by making the asset checks loader return AssetsDefinitions, and changing the assertions to just make sure that the returned assets only contain checks.

How I Tested These Changes

Altered existing tests

[dpeng817]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [module-loaders] Further simplify asset checks path #26527 👈 (View in Graphite)
• [module-loaders] Delete extra_source_assets param from assets_from_modules #26494 : 9 other dependent PRs (#26484 , #26498 , #26514 and 6 others)
• master

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

[OwenKephart]

I thought about this a bit more, and as much as it really is unlikely that this causes issues, it is part of our public api and we should do our best to respect semver. These bits of code are ugly but not necessarily an ongoing maintenance concern

[yuhan]

The return type of load_asset_checks_from_modules has been changed from Sequence[AssetChecksDefinition] to Sequence[AssetsDefinition]. AssetChecksDefinition is a thin wrapper around AssetsDefinition, so we don't anticipate this should require any code behavior changes on the part of our users.

"AssetChecksDefinition is a thin wrapper around AssetsDefinition" predates this PR, but i actually think exposing this implementation is a little risky as many users may not think asset check is a thin wrapper around asset definition (if you look at our doc today, asset_check feels like a peer concept to asset than a extending implementation.

with that, i'm with owen that while this isn't a significant change, it could be a thrash to existing users. for example, a concrete confusing case could be:

schedules: Sequence[ScheduleDefinition] = ...
sensors: Sequence[SensorDefinition] = ...
# one may question why this returns a list of AssetsDefinition's but still need to be provided separately 
asset_checks: Sequence[AssetsDefinition]  = load_asset_checks_from_modules([checks_module])
# ok...this assets type hint is a separate issue that we worry about later...
assets: Sequence[Union[AssetsDefinition, SourceAsset, CacheableAssetsDefinition]] = load_assets_from_modules([assets_modle])

defs = Definitions(
    assets=assets,
    asset_checks=asset_checks,
    schedules=schedules,
   sensors=sensors,
)

[dpeng817]

Perhaps an alternative could be an easy transformation API that coerces an AssetsDefinition to an AssetChecksDefinition, assuming the properties line up. That way we don't have to have this boilerplate in multiple places, at least

switched to a coercion method on AssetsDefinition

[graphite-app]

Graphite Automations

"Add a 'docs-to-migrate' label to PRs with docs" took an action on this PR • (12/18/24)

1 label was added to this PR based on Christopher DeCarolis's automation.

addressed

[yuhan]

make sure you remove the changelog section which seems no longer valid.