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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
140

Not exactly, right?

159

these docs are off

176

does this yield a useful error msg?

python_modules/dagster/dagster/utils/yaml_utils.py
60

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

85

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
140

Yeah you're right, will update this text

159

argh, I missed this. thanks for catching.

176

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
60

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

85

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
57

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
57

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.