Page MenuHomeElementl

[dagstermill] Allow for tags & description to be passed into solids (issue 3290)
ClosedPublic

Authored by sidkmenon on Jan 25 2021, 5:01 PM.

Details

Summary

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.

Test Plan

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

Repository
R1 dagster
Branch
issue-3290 (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

rexledesma added inline comments.
python_modules/libraries/dagstermill/dagstermill/solids.py
320–322

nit: f-strings

catherinewu added inline comments.
python_modules/libraries/dagstermill/dagstermill/solids.py
299

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)

(see: https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_tags.py#L4)

This revision now requires changes to proceed.Jan 25 2021, 6:30 PM
sidkmenon marked 2 inline comments as done.

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)

Awesome, looks great. one last change (and a missing .) and then it's good to ship

python_modules/libraries/dagstermill/dagstermill/solids.py
298
329

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

This revision now requires changes to proceed.Jan 26 2021, 12:59 AM
sidkmenon marked 2 inline comments as done.

Addressing comments from @catherinewu

  1. Adding period to end of 'description' in python documentation on solids.py::299
  2. Using validate_tags as per @catherinewu's comment instead of check_opt_dict
  3. Refactored reserved tag check on solids.py::325 (approx.) because it was a bit too repetitive
catherinewu added inline comments.
python_modules/libraries/dagstermill/dagstermill/solids.py
326

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

This revision now requires changes to proceed.Jan 26 2021, 2:50 AM

Removing inline function definition

This revision is now accepted and ready to land.Jan 26 2021, 6:35 AM