Page MenuHomePhabricator

Evaluate config mapping when solids config is empty
ClosedPublic

Authored by max on Tue, Nov 19, 1:53 AM.

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.Tue, Nov 19, 1:53 AM
max removed a reviewer: Restricted Project.Tue, Nov 19, 2:53 AM
max removed a reviewer: schrockn.

this seems a touch fishy to have to special case the solid dict in this way - I dont know the config system well enough to hazard a guess at another approach. @natekupp ?

python_modules/dagster/dagster/core/types/evaluator/evaluation.py
157โ€“158

๐Ÿ˜‰

nate requested changes to this revision.Wed, Nov 20, 7:11 PM

I'm fine with the overall thing here since we're already doing a similar kind of check for SolidContainerConfigDict - but see comments on consistency

python_modules/dagster/dagster/core/definitions/environment_configs.py
82

name consistent with SolidContainerConfigDict since it serves a similar purpose?

101

given that is_solid_container_config does an instance check vs. attr check, can we make these consistent?

This revision now requires changes to proceed.Wed, Nov 20, 7:11 PM
max added inline comments.Wed, Nov 20, 7:21 PM
python_modules/dagster/dagster/core/definitions/environment_configs.py
101

possibly, we will have to introduce another intermediate class i think

python_modules/dagster/dagster/core/types/evaluator/evaluation.py
157โ€“158

thx

max updated this revision to Diff 6860.Mon, Nov 25, 6:29 PM

Nits from review

max updated this revision to Diff 6862.Mon, Nov 25, 6:55 PM

Blk and nits

nate accepted this revision.Mon, Nov 25, 7:39 PM

thanks for fixing this!

This revision is now accepted and ready to land.Mon, Nov 25, 7:39 PM
max updated this revision to Diff 6869.Mon, Nov 25, 7:49 PM

Rebase