Page MenuHomePhabricator

Add PresetDefinition.from_pkg_resources
ClosedPublic

Authored by nate on Apr 4 2020, 6:37 PM.

Details

Summary

Fixes #2349.

https://github.com/dagster-io/dagster/issues/2349

This adds an API PresetDefinition.from_pkg_resources(). It works both for file-based (no pip install) and module-based preset loads.

I moved over the airline demo presets in this diff to give a concrete example, but once landed should follow up and move over all dagster_examples module presets.

Test Plan

unit

Diff Detail

Repository
R1 dagster
Branch
add_yaml_str
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

nate created this revision.Apr 4 2020, 6:37 PM
nate edited the summary of this revision. (Show Details)Apr 4 2020, 6:42 PM
nate edited the summary of this revision. (Show Details)
nate edited the summary of this revision. (Show Details)Apr 4 2020, 8:03 PM
nate retitled this revision from Add PresetDefinition.from_yaml_strings to Add PresetDefinition.from_pkg_resources.Apr 4 2020, 8:10 PM
max requested changes to this revision.Apr 5 2020, 4:11 AM

i think there are some transpositions in the docstrings

python_modules/dagster/dagster/core/definitions/preset.py
138

Not exactly, right?

157

these docs are off

174

does this yield a useful error msg?

python_modules/dagster/dagster/utils/yaml_utils.py
45

same q as below -- here i feel like we can do even better and enumerate, so we can look up the bad filename

70

is this a practically useful error msg? would it be more helpful to print only the bad yaml dict?

This revision now requires changes to proceed.Apr 5 2020, 4:11 AM
nate marked 5 inline comments as done.Apr 5 2020, 5:21 PM

Cool, thank you for taking a close look! Updated to address comments.

python_modules/dagster/dagster/core/definitions/preset.py
138

Yeah you're right, will update this text

157

argh, I missed this. thanks for catching.

174

yes, the pkg_resource call that fails to load will raise an error w/ info on what's wrong—but I've added a six.raise_from to match the other staticmethods here and tests for a few different bad cases just for good measure

python_modules/dagster/dagster/utils/yaml_utils.py
45

yep I agree, added this enumeration and print the bad filename. I also added a test case covering both of these

70

yes agree, updated

nate updated this revision to Diff 11435.Apr 5 2020, 5:21 PM
nate marked 5 inline comments as done.

up

max accepted this revision.Apr 6 2020, 3:39 AM
max added inline comments.
python_modules/dagster/dagster_tests/utils_tests/test_yaml.py
56

there is some risk of leaking secrets in these error msgs

This revision is now accepted and ready to land.Apr 6 2020, 3:39 AM
nate added inline comments.Apr 6 2020, 1:55 PM
python_modules/dagster/dagster_tests/utils_tests/test_yaml.py
56

yeah, good point. I guess you'd have to (1) not be using StringSource, and (2) have an invalid YAML document which contains secrets... seems acceptable for now?

This revision was automatically updated to reflect the committed changes.