Page MenuHomeElementl

added parameter to default fxn for dagster type loader version.
ClosedPublic

Authored by cdecarolis on Sep 11 2020, 5:15 PM.

Details

Summary

added param to default fxn

Test Plan

na

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Mind adding a test for this?

This revision now requires changes to proceed.Sep 11 2020, 5:45 PM

Added tests, and made default computed version None (we're using None as our sentinel value for no versions provided).

python_modules/dagster/dagster/core/types/config_schema.py
80–82

can we add type checking here? i don't see these two params being checked in the stack.

101

do we assume versions will always be provided in memoized run? if so, it may be better to error out instead of a silent None. if not, can we comment some cases which will hit this path?

python_modules/dagster/dagster_tests/core_tests/runtime_types_tests/config_schema_tests/test_config_schema.py
72

would it be worth making this attribute public? i havent read through D4407 tho. it's your call.

This revision is now accepted and ready to land.Sep 12 2020, 9:13 PM
python_modules/dagster/dagster/core/types/config_schema.py
80–82

good call, will do.

101

I don't think we should always assume that versions will be provided. The way I was thinking about it was that None provides a mechanism for specifying that a specific portion of a pipeline should re-run every time. The reason I think this would be okay is that even though the None is technically silent, the version information would presumably be populated in dagit/cli, and the user would still be able to see that this loading code is running every time, for example.

python_modules/dagster/dagster_tests/core_tests/runtime_types_tests/config_schema_tests/test_config_schema.py
72

it's not used outside of the computation of version for DagsterTypeLoader, but I could see it being used for populating version information later, so I think it might be worth making it public.

Made loader version a public property

Added public loader_version property.

This revision is now accepted and ready to land.Sep 14 2020, 7:04 PM