Page MenuHomePhabricator

Full support for dagster.Set and dagster.Tuple
ClosedPublic

Authored by max on Fri, Nov 1, 5:20 PM.

Details

Reviewers
alangenfeld
schrockn
Group Reviewers
Restricted Project
Commits
R1:c2a3b54e1e66: Full support for dagster.Set and dagster.Tuple
Summary

Makes Set and Tuple importable from dagster and adds input hydration configs.

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

max created this revision.Fri, Nov 1, 5:20 PM

Can you only use Set/Tuple in config for input hydration but not for config schema? I don't quite get why ConfigTuple doesn't enable that case

docs/sections/api/apidocs/test_type_examples.py
3–27

nit: consider using using pytest.raises(..., match=) instead of assert in exc (the test failure messages are more clear that way in my opinion)

python_modules/dagster/dagster/core/definitions/pipeline.py
479–481

just use display name here

python_modules/dagster/dagster/core/types/__init__.py
18–30

nit: feel like Set,Tuple,List,Dict should live together

max added inline comments.Fri, Nov 1, 5:46 PM
docs/sections/api/apidocs/test_type_examples.py
3–27

for reasons i don't understand, match frequently doesn't work with long matches such as these

python_modules/dagster/dagster/core/definitions/pipeline.py
479–481

yep

python_modules/dagster/dagster/core/types/__init__.py
18–30

hmm, the implementations are wildly different

schrockn added inline comments.Fri, Nov 1, 5:51 PM
docs/sections/api/apidocs/test_type_examples.py
3–27

It's because it passes its value directly to re.match.

3–27

docs says:

To match a literal string that may contain special characters, the pattern can first be escaped with re.escape.
max added inline comments.Fri, Nov 1, 6:01 PM
docs/sections/api/apidocs/test_type_examples.py
3–27

aha

max updated this revision to Diff 6164.Fri, Nov 1, 10:45 PM

Rebase

max updated this revision to Diff 6180.Mon, Nov 4, 5:14 PM

Rebase

Reframing my question from before that might have been missed - should there be a test for config_field=Field(Tuple[String, String]) or is that not supported for a reason I don't see

max updated this revision to Diff 6199.Mon, Nov 4, 9:53 PM

Support for Set and Tuple in config values

This revision is now accepted and ready to land.Mon, Nov 4, 9:56 PM
This revision was landed with ongoing or failed builds.Mon, Nov 4, 10:44 PM
This revision was automatically updated to reflect the committed changes.