Incorrect return type of functions in load_assets_from_modules

[emirkmo]

Dagster version

1.7.4

What's the issue?

The helper functions to load assets in load_assets_from_modules all return Sequence[Union[A, B, C]] (where [A, B, C] stands for AssetsDefinition, SourceAsset, Cacheable...). However, these functions explicitly return a list not a generic Sequence.

For type annotations, concrete functions/classes should specify the actual type of what they return. In this case, these functions all returns lists, NOT a generic Sequence.

The confusion might have arisen from the fact that, for appropriate generality in a library like Dagster, functions and classes should be as generic as possible in what they accept as input but be as specific as possible in what they return as output.

The helper functions in this module all returns lists. So it is safe to change them to their actual return type of list. As lists, they are still Sequence[Union[A, B, C], and will naturally pass the type check for any functions that expects input of a generic Sequence[Union[A, B ,C]. But are also more concretely List[Union[A, B, C] and should be typed as such so that one can actually use the returned objects in their own user code.

Examples of the issues:

If I try to append or manipulate the returns, they have the incorrect type of Sequence.
A sequence does not have .append for instance. It
cannot be concatenated with a list of AssetDefinitions,
etc. If using strict type checking, which we do, I
cannot even easily combine the output of these functions
with assets imported "manually". I would get the error
list[AssetsDefinition] is incompatiable with Sequence[...].

It is quite annoying to have to typing.cast the output
of library code from Dagster into its actual type just to
get my own type checking to work. It is made harder by the
fact that CacheableAssetsDefintion is not exported by default,
so I have to import it, and cast the output to
list[AssetsDefinition | SourceAsset | Cacheable...]].

What did you expect to happen?

No response

How to reproduce?

No response

Deployment type

None

Deployment details

No response

Additional information

No response

Message from the maintainers

Impacted by this issue? Give it a 👍! We factor engagement into prioritization.