Page MenuHomeElementl

[crag] single config parameter to to_job
ClosedPublic

Authored by sandyryza on Wed, Jun 30, 3:27 PM.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sandyryza retitled this revision from crag config revamp to single config parameter to to_job.Wed, Jun 30, 3:28 PM
sandyryza added reviewers: alangenfeld, cdecarolis, max.
sandyryza retitled this revision from single config parameter to to_job to [crag] single config parameter to to_job.
Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Jun 30, 3:54 PM
Harbormaster failed remote builds in B32897: Diff 40542!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jul 1, 4:45 PM
Harbormaster failed remote builds in B32989: Diff 40660!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jul 1, 5:23 PM
Harbormaster failed remote builds in B32992: Diff 40664!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jul 1, 6:11 PM
Harbormaster failed remote builds in B32999: Diff 40674!

overall support this direction - leaving open for feedback from others

is the schedule apis coming as a diff on top of this? curious to see how that looks

python_modules/dagster/dagster/core/definitions/graph.py
381–382

re: my favorite thing to bring up around this config, will want to communicate here some how

  • this is loadable in dagit as plain text so don't use any sensitive values like passwords
  • you can use a config mapping with no outer schema to set config in a not viewable way
python_modules/dagster/dagster/core/definitions/partition.py
621–623

nit: PartitionsConfig reads a little odd to me. If users are unlikely to construct directly not sure how much it matters but throwing out some alternatives

  • PartitionDrivenConfig
  • PartitionedConfig
  • PartitionConfigMapping
  • PartitionedConfigMapping
python_modules/dagster/dagster_tests/core_tests/graph_tests/test_graph.py
170–203

a little unfortunate to lose this capability, seems recoverable if feedback pushes us that way

q mgmt - inline comments, get green

This revision now requires changes to proceed.Fri, Jul 2, 9:45 PM
python_modules/dagster/dagster/core/definitions/graph.py
381–382

Totally. I added some warnings about putting in sensitive values. I'm a little hesitant to say "you can use a config mapping with no outer schema to set config in a not viewable way", because I think it would be valuable at some point in the future to expose these inner config values for debugging.

IMO config is just not the right place for sensitive information. Maybe worth a bigger conversation?

python_modules/dagster/dagster/core/definitions/partition.py
621–623

Yeah, me too. I changed to PartitionedConfig.

python_modules/dagster/dagster/core/definitions/mode.py
56–93

just collapse these two in to one _config: Optional[Union[...]] ?

This revision is now accepted and ready to land.Tue, Jul 6, 6:17 PM
python_modules/dagster/dagster/core/definitions/mode.py
56–93

I just tried playing with this and I think it's a little awkward, because it still leaves preset out. I'm going to merge without it, but can make the change if you feel strongly.

This revision was automatically updated to reflect the committed changes.