Page MenuHomePhabricator

RFC use custom yaml loader
Needs ReviewPublic

Authored by alangenfeld on Thu, Nov 12, 10:56 PM.

Details

Summary

If someone has a yaml file with a datestring like 2020-01-01 , it gets parsed in to a datetime object which will fail when it hits our serdes libraries.

I think if we want to support datetime object creation, it sould be part of the config system's post processing, not the yaml step.

This diff changes our yaml utils to use a custom child of SafeLoader which drops this datetime resolution.

resolves #2654

Test Plan

added test, can add more

Diff Detail

Repository
R1 dagster
Branch
yaml (branched from master)
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningpython_modules/dagster/dagster_tests/general_tests/utils_tests/test_yaml.py:117W0612Unused Variable
Unit
No Unit Test Coverage

Event Timeline

So I guess the question is, would users expect these to be interchangeable?

preset = PresetDefinition.from_files(['file.yaml'])
preset = PresetDefinition(yaml.safe_load(open('file.yaml')))

I think maybe? How difficult is handling this at the serdes level?

To your queue

This revision now requires changes to proceed.Mon, Nov 16, 3:43 PM

I think maybe?

That makes sense - but then you are going to fail config validation unless this is in a part of the config that is unchecked like in a Permissive dict.

My intuition was that it would be weird to get a datetime object in your solid_config , but that also comes from a place of being surprised that yaml parsing automatically coerces certain strings in to datetime objects.

Should this diff come with a datetime oriented config type? Would that type allow datetime objects at input time as well as parsing strings in to datetime in post process?

requesting review to gather more thoughts

How difficult is handling this at the serdes level?

very doable if we want to go that path

My intuition was that it would be weird to get a datetime object in your solid_config , but that also comes from a place of being surprised that yaml parsing automatically coerces certain strings in to datetime objects.

Yeah, I definitely agree this is weird.

I don't think I have a strong opinion either way.

What's the error message, or does parsing just proceed without the datetime being parsed? YAML is complex enough that I can imagine someone tearing their hair out about this behavior with no clue how to proceed..