[module-loaders] Genericize object list functionality to take in all sensors, jobs, schedules

[dpeng817]

Summary & Motivation

Actually genericize ModuleScopedDagsterObjects to handle taking in all types of Dagster objects - sensors, schedules, and jobs. I explicitly left out resources and loggers, because I don't think it makes sense to scoop those up at module load - since they need to be associated with a key.

How I Tested These Changes

Added a new test file test_module_loaders which scaffolds out a module fake for a given test spec, and ensures that either an error is thrown or the correct number of attributes are accessible on the underlying list.

[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] Genericize object list functionality to take in all sensors, jobs, schedules #26545 👈 (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]

This structure looks pretty annoying to maintain.

I've run into this annoyance myself, but it does look like get_args(Union[x, y, z]) (imported from typing) would return a tuple of (x, y, z), which we could use for the runtime type checking, which would eliminate this sort of duplication

[dpeng817]

ah nice. That seems acceptable to me.

[dpeng817]

just to follow up on this; I think that the get_args approach doesn't actually work with type introspection in pylance; so I think this solution is no longer acceptable. I agree that it's annoying to maintain but I don't think it's that annoying

[OwenKephart]

maybe:

LoadableAssetDef
LoadableScheduleDef
LoadableJobDef

?

[dpeng817]

LoadableAssetDef I think is particularly confusing given that it can be a SourceAsset or AssetSpec, for example

addressed comments

[OwenKephart]

I think we should abandon this RuntimeObjectType / LoadableXObject conceptual thing and just write things out explicitly for now.

This buys us a few saved lines of code at the cost of two new conceptual categories that need to be kept in sync with eachother anyway.

[OwenKephart]

If we can't do this automatically then I would much prefer putting the types here explicitly rather than importing the type tuple from elsewhere.

So that would at least be replacing RuntimeScheduleObjectTypes with the explicit (ScheduleDefinition, UnresolvedPartitionedAssetScheduleDefinition)

I also think ScheduleDefinitionObject is confusing enough of a name that we should probably just put the Union[ScheduleDefinition, UnresolvedPartitionAssetScheduleDefinition] there as well.

This is actually not any more redundant than the current solution and localizes the redundancy so it's easier to wrap your head around (and doesn't require the creation of a new concept).

The only place where this adds more LoC is the LoadableDagsterObject bit (where we do need to re-list all of these types), which I think is acceptable.

I'd rather spend the lines of code and keep everything in one file.

[dpeng817]

fair enough