[module-loaders] [rfc] Definitions from module loader

[dpeng817]

Summary & Motivation

This PR is a prototype of a top-level API to load dagster definitions from across a module into a single returned Definitions object. It is not intended for landing yet. Just want to align on direction.

Why add this API?

For any user who is bought into a module structure making use of load_assets_from_x, we are currently making their life unnecessarily more difficult by not bringing in other objects that are scoped at module-load time. Those users agree - we've received requests for an API to do that numerous times.

In absence of a compelling replacement for our current project structure, I think the existence of this API is a good stopgap to improve module ergonomics.

Why are resources, loggers, and executor provided as args?

It seems like the most straightforward way to support these objects without some sort of additional magic. Since we force you to provide a key in addition to the class itself, there's not currently a module-scoped pattern that matches how these objects are defined on a Definitions argument. So it seems reasonable to accept these as parameters.

The other approach would be to allow users to configure resources as variables and use the key as a variable. But I figure the more conservative approach here leaves us room to also do that in the future (can imagine some sort of combination step).

dbt = DbtCliResource(...)
# equivalent of
{"dbt": DbtCliResource(...)}

For executor, the fact that we only accept one per module is kind of unique. I don't think a user would define an executor in the same way that they define a sensor, schedule, or job, for example, where the definition is inlined to the module. I think it makes more sense for the user to need to provide this explicitly.

Why does this return a Definitions object

I don't think there is any reasonable alternative. The existing load_assets_from_x is nice because that whole list can be provided directly to a Definitions object - this is not the case for if we provided a flat list of heterogeneous types of Dagster objects - this could not be provided directly to a Definitions object. Unless we added some sort of from_list constructor or something. But I think this is more straightforward.

It also gives us the opportunity to potentially paper over some stuff; if we so choose - we can, for example, automatically coerce source assets and AssetSpec objects into resolved AssetsDefinitions.

Should this thing take in other Definitions objects?

Right now, I think no. While Definitions.merge exists, it's obscured and not documented, I think most users think of a single Definitions object as being synonymous with a code location.

What's the intended design pattern for using this?

I think the intended use case for this would be to provide one call at the top level of the module of a given code location; and automatically load in all of your dagster defs.

defs = load_definitions_from_module(current_module, resources=..., loggers=...)

How I Tested These Changes

I added a new test which operates on all test specs and calls this fxn, and also one for handling the resource and logger cases.

[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] [rfc] Definitions from module loader #26546 👈 (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.

[schrockn]

I think this an excellent change. We can use this in components as well. cc: @OwenKephart

[schrockn]

I also think passing resources in like this is a good compromise.

[yuhan]

Approving to unblock the landing. Since this is structured as an internal API, I don’t think the stakes are high enough for us to bikeshed on the names forever.

A couple of open discussions in this stack:

• [edited] I had another open question but on a second thought, I think passing resources is smart, esp in a vary-resources-per-env world, you wouldn't want to blindly load every resource.
  • (Excluding loggers makes sense to me. As a side note, I almost wonder if we could long-term move loggers to be more devops-y - perhaps as part of Components - rather than at the code location definitions level. But this is just a separate note that doesn't have to be addressed right now)
• Naming Separate Pandas Dataframe Solid into Two Sources #1: load_definitions_from_module vs load_defs_from_module: I think definitions is clearer and more descriptive, and it matches load_assets_. just simply full name.
• Naming Purge qhp from git history #2: Type hints: LoadableAssetObject vs LoadableAssetDef: I vote for Def. I don’t think users care about the implementation details that Def may not equal Union[AssetsDef, AssetSpec, SourceAsset] and neither would I. To me, they all fall under the umbrella of Def, similar to JobDef, ScheduleDef, etc.

[schrockn]

@dpeng817 when is this getting into master?

[dpeng817]

@schrockn ideally today. There's been a ton of test failures to chase down. But I'll be sure to let you know when it does