Depends on D8612.
Details
Details
bk
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Branch
- crag-config (branched from master)
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Comment Actions
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 | ||
|---|---|---|
| 367–368 | re: my favorite thing to bring up around this config, will want to communicate here some how
| |
| python_modules/dagster/dagster/core/definitions/partition.py | ||
| 624–626 | 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
| |
| python_modules/dagster/dagster_tests/core_tests/graph_tests/test_graph.py | ||
| 163–196 | a little unfortunate to lose this capability, seems recoverable if feedback pushes us that way | |
| python_modules/dagster/dagster/core/definitions/graph.py | ||
|---|---|---|
| 367–368 | 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 | ||
| 624–626 | 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[...]] ? | |
| 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. | |