Skip to content

Commit

Permalink
replace "__dagster_no_value" with the empty string (#20707)
Browse files Browse the repository at this point in the history
## Summary & Motivation

I'm not 100% convinced this is the right direction, but this PR is meant
move forward the discussion from this slack thread:
https://dagsterlabs.slack.com/archives/C04PD3G2F7H/p1711139737671339.

It makes the case for replacing "__dagster_no_value" with the empty
string.

Why the empty string vs. `None` or `True`? A couple main reasons:
- Avoids backcompat issues from expanding the set of types tags can have
- More straightforward to build UI for inputting
- Docs come out simplest


### Avoids backcompat issues from expanding the set of types tags can
have

- Ideally, we'd like to have a single way of handling tags across the
system, i.e. run tags and op tags should work the same as asset tags.
- Run tag and op tag values are currently strings (dicts can be supplied
to tags arguments, but they're coerced to strings).
- Supporting non-string run & op tag values would be a breaking change,
because public methods like OpDefinition.tags would be able to return a
different type.

### More straightforward to build UI for inputting

We currently have the ability to run tags in the UI, and we'll probably
want to expand this in the future to asset tags at some point.

<img width="732" alt="image"
src="https://github.com/dagster-io/dagster/assets/654855/559f30c9-fad5-43c1-ae3b-4b594de4e6d0">

If `True` or `None` have special meaning, then we need to worry about
users' expectations when they input those strings directly. I suspect
there will be a non-negligible set of users who input tags both in the
UI and in code.

### Docs come out the simplest

'Tags are key/value pairs, where the keys are strings and the values are
either strings or boolean `True`/`False`. When a tag’s value is True,
only the key will be shown in the UI. E.g. `{"PII": True}` will render
as just “PII”. When the value is False, the tag will be ignored.'

vs.

'Tags are key/value pairs of strings. If a tag’s value is the empty
string, only the key will be shown in the UI. E.g. `{"PII": ""}` will
render as just “PII”.'
  • Loading branch information
sryza authored Mar 26, 2024
1 parent ebf073f commit b13e294
Show file tree
Hide file tree
Showing 7 changed files with 7 additions and 22 deletions.
4 changes: 1 addition & 3 deletions js_modules/dagster-ui/packages/ui-core/src/ui/tagAsString.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
const TAG_NO_VALUE_SENTINEL = '__dagster_no_value';

export const buildTagString = ({key, value}: {key: string; value: string}) => {
if (value === TAG_NO_VALUE_SENTINEL) {
if (value === '') {
return key;
}
return `${key}: ${value}`;
Expand Down
1 change: 0 additions & 1 deletion python_modules/dagster/dagster/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,6 @@
from dagster._core.storage.tags import (
MAX_RUNTIME_SECONDS_TAG as MAX_RUNTIME_SECONDS_TAG,
MEMOIZED_RUN_TAG as MEMOIZED_RUN_TAG,
TAG_NO_VALUE as TAG_NO_VALUE,
)
from dagster._core.storage.upath_io_manager import UPathIOManager as UPathIOManager
from dagster._core.types.config_schema import (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
fetch_sources,
parse_clause,
)
from dagster._core.storage.tags import TAG_NO_VALUE
from dagster._model import DagsterModel
from dagster._serdes.serdes import whitelist_for_serdes

Expand Down Expand Up @@ -197,9 +196,7 @@ def tag_string(string: str, include_sources: bool = False) -> "AssetSelection":
"""
split_by_equals_segments = string.split("=")
if len(split_by_equals_segments) == 1:
return TagAssetSelection(
key=string, value=TAG_NO_VALUE, include_sources=include_sources
)
return TagAssetSelection(key=string, value="", include_sources=include_sources)
elif len(split_by_equals_segments) == 2:
key, value = split_by_equals_segments
return TagAssetSelection(key=key, value=value, include_sources=include_sources)
Expand Down
3 changes: 0 additions & 3 deletions python_modules/dagster/dagster/_core/storage/tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@
]


TAG_NO_VALUE = "__dagster_no_value"


class TagType(Enum):
# Custom tag provided by a user
USER_PROVIDED = "USER_PROVIDED"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
from dagster._core.definitions.assets import AssetsDefinition
from dagster._core.definitions.base_asset_graph import BaseAssetGraph
from dagster._core.definitions.events import AssetKey
from dagster._core.storage.tags import TAG_NO_VALUE
from dagster._serdes import deserialize_value
from dagster._serdes.serdes import _WHITELIST_MAP
from pydantic import ValidationError
Expand Down Expand Up @@ -712,7 +711,7 @@ def test_deserialize_old_all_asset_selection():

def test_from_string_tag():
assert AssetSelection.from_string("tag:foo=bar") == AssetSelection.tag("foo", "bar")
assert AssetSelection.from_string("tag:foo") == AssetSelection.tag("foo", TAG_NO_VALUE)
assert AssetSelection.from_string("tag:foo") == AssetSelection.tag("foo", "")


def test_tag():
Expand All @@ -738,8 +737,8 @@ def test_tag_string():
AssetSpec("asset2", tags={"foo": "fooval2"}),
AssetSpec("asset3", tags={"foo": "fooval", "bar": "barval"}),
AssetSpec("asset4", tags={"bar": "barval"}),
AssetSpec("asset5", tags={"baz": TAG_NO_VALUE}),
AssetSpec("asset6", tags={"baz": TAG_NO_VALUE, "bar": "barval"}),
AssetSpec("asset5", tags={"baz": ""}),
AssetSpec("asset6", tags={"baz": "", "bar": "barval"}),
]
)
def assets(): ...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
check_opt_coercible_to_asset_key_prefix_param,
)
from dagster._core.definitions.utils import is_valid_definition_tag_key
from dagster._core.storage.tags import TAG_NO_VALUE

from .asset_utils import (
default_asset_key_fn,
Expand Down Expand Up @@ -250,7 +249,7 @@ def get_tags(self, dbt_resource_props: Mapping[str, Any]) -> Mapping[str, str]:
return {"custom": "tag"}
"""
tags = dbt_resource_props.get("tags", [])
return {tag: TAG_NO_VALUE for tag in tags if is_valid_definition_tag_key(tag)}
return {tag: "" for tag in tags if is_valid_definition_tag_key(tag)}

@public
def get_group_name(self, dbt_resource_props: Mapping[str, Any]) -> Optional[str]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
)
from dagster._core.definitions.utils import DEFAULT_IO_MANAGER_KEY
from dagster._core.execution.context.compute import AssetExecutionContext
from dagster._core.storage.tags import TAG_NO_VALUE
from dagster._core.types.dagster_type import DagsterType
from dagster_dbt.asset_decorator import DUPLICATE_ASSET_KEY_ERROR_MESSAGE, dbt_assets
from dagster_dbt.core.resources_v2 import DbtCliResource
Expand Down Expand Up @@ -623,10 +622,7 @@ def test_dbt_config_tags(test_meta_config_manifest: Dict[str, Any]) -> None:
@dbt_assets(manifest=test_meta_config_manifest)
def my_dbt_assets(): ...

assert my_dbt_assets.tags_by_key[AssetKey("customers")] == {
"foo": TAG_NO_VALUE,
"bar-baz": TAG_NO_VALUE,
}
assert my_dbt_assets.tags_by_key[AssetKey("customers")] == {"foo": "", "bar-baz": ""}
for asset_key in my_dbt_assets.keys - {AssetKey("customers")}:
assert my_dbt_assets.tags_by_key[asset_key] == {}

Expand Down

1 comment on commit b13e294

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-oh7b1gt62-elementl.vercel.app

Built with commit b13e294.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.