Change solids.py::define_dagstermill_solids to allow for custom descriptions & tags. Default tags & descriptions are the same as before, unless explicitly written over by user input. See https://github.com/dagster-io/dagster/issues/3290 for more context.
Details
Details
Added tests for custom/default tags & descriptions, respectively. For tags, checked that 1) default tags were present (didn't test overwriting them bc. I can't imagine a case where the user would want to change the "kind" : "ipynb" mapping for a notebook, for example). I also checked 2) that custom tags were present when added. For descriptions, I checked that the default description was present unless a custom description was passed in.
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| python_modules/libraries/dagstermill/dagstermill/solids.py | ||
|---|---|---|
| 321–323 | nit: f-strings | |
| python_modules/libraries/dagstermill/dagstermill/solids.py | ||
|---|---|---|
| 300 | We should document this decision in the doc string. We can also check the tags dict w/ something like the following: check.invariant(notebook_path not in tags, 'user-defined solid tags contains notebook_path key, but notebook_path key is reserved for use by dagster') | |
| python_modules/libraries/dagstermill/dagstermill_tests/test_solids.py | ||
| 355 | Interesting. This works, and I think we should test the solid tags directly as well (easier to read + maintain) | |
Comment Actions
Fix test structure & check that default tags are not overridden
Tests: Use define_dagstermill_solid function directly to simplify test structure; get rid of old pipeline tests (except for one to make sure that pipelines still run with custom tags & descriptions).
Tags: use check library to make sure that default tags ('kind' and 'notebook_path' are not overridden by the user)
Comment Actions
Awesome, looks great. one last change (and a missing .) and then it's good to ship
| python_modules/libraries/dagstermill/dagstermill/solids.py | ||
|---|---|---|
| 299 | ||
| 330 | we should use validate_tags which both checks the opt_dict_param part but also verifies that the json can be stringified. from dagster.core.definitions.utils import validate_tags | |
Comment Actions
Addressing comments from @catherinewu
- Adding period to end of 'description' in python documentation on solids.py::299
- Using validate_tags as per @catherinewu's comment instead of check_opt_dict
- Refactored reserved tag check on solids.py::325 (approx.) because it was a bit too repetitive
| python_modules/libraries/dagstermill/dagstermill/solids.py | ||
|---|---|---|
| 327 | hmm, we usually don't nest functions within control flow. having a lambda is borderline, but would prefer to move this up a level or two | |
