sentinel for tags without values #20388
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
sryza
force-pushed
the
asset-selection-tags
branch
from
bb66632 to
cbc1643
Compare
schrockn
reviewed
Mar 11, 2024
Yeah I've gone back and forth on this and didn't reach a clear answer. I started with the string sentinel because it was easier to implement - doesn't require changing types everywhere. Going with a non-string sentinel would send us in the direction of accepting that on run tags as well, which seemed like it could have downstream implications. Could definitely be convinced either way. |
sryza
force-pushed
the
asset-selection-tags
branch
from
cbc1643 to
c7ef26e
Compare
sryza
added a commit
that referenced
this pull request
Mar 12, 2024
## Summary & Motivation This PR passes tags up through graphql to the UI and displays them on the asset overview page. It doesn't yet support the no-value tags introduced in this PR: #20388. Stacks on top of #20351. <img width="1167" alt="image" src="https://github.com/dagster-io/dagster/assets/654855/97bcb447-06e6-4cd1-a0f2-4fdfffdd1d0d"> ## How I Tested These Changes
sryza
changed the base branch from
asset-selection-tags
to
tags-asset-selection
sryza
force-pushed
the
tag-no-value
branch
2 times, most recently
from
3b5f6af to
a1230f2
Compare
sryza
force-pushed
the
tags-asset-selection
branch
from
7673295 to
35cdf77
Compare
sryza
force-pushed
the
tags-asset-selection
branch
from
35cdf77 to
903a174
Compare
|
I'm ok with the string version of this, given that runless event tags are also strings rather than objects. I can't see any major benefits to making it an object sentinel instead. That being said, looks like our APIs only accept dict mappings right now, so where will this be used? Is the intent to automatically ingest dbt tags and punt on python APIs? |
sryza
force-pushed
the
tags-asset-selection
branch
from
903a174 to
cdde23f
Compare
Good question. I think there three stages here:
|
branch-name: tag-no-value
|
@sryza that makes sense to me, I'm comfortable with (1) and (2). |
clairelin135
approved these changes
Mar 19, 2024
benpankow
pushed a commit
that referenced
this pull request
Mar 21, 2024
## Summary & Motivation
Adds a special sentinel value for tags that indicates "this tag is just
a string, not a key-value pair". How this sentinel will be used:
- In the CLI, when a selection like `--select tag:foo` is provided,
we'll interpret it as `AssetSelection.tag("foo", "__dagster_novalue")`
(implemented in this PR)
- The dbt integration will provide this value for all tags (because dbt
tags are strings).
- In the UI, we'll hide this sentinel value. I.e. a tags dict like
`{"foo": "fooval", "bar": "__dagster_novalue"}` will be rendered as
"foo=fooval, bar" in the UI.
## How I Tested These Changes
PedramNavid
pushed a commit
that referenced
this pull request
Mar 28, 2024
## Summary & Motivation This PR passes tags up through graphql to the UI and displays them on the asset overview page. It doesn't yet support the no-value tags introduced in this PR: #20388. Stacks on top of #20351. <img width="1167" alt="image" src="https://github.com/dagster-io/dagster/assets/654855/97bcb447-06e6-4cd1-a0f2-4fdfffdd1d0d"> ## How I Tested These Changes
PedramNavid
pushed a commit
that referenced
this pull request
Mar 28, 2024
## Summary & Motivation
Adds a special sentinel value for tags that indicates "this tag is just
a string, not a key-value pair". How this sentinel will be used:
- In the CLI, when a selection like `--select tag:foo` is provided,
we'll interpret it as `AssetSelection.tag("foo", "__dagster_novalue")`
(implemented in this PR)
- The dbt integration will provide this value for all tags (because dbt
tags are strings).
- In the UI, we'll hide this sentinel value. I.e. a tags dict like
`{"foo": "fooval", "bar": "__dagster_novalue"}` will be rendered as
"foo=fooval, bar" in the UI.
## How I Tested These Changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
Adds a special sentinel value for tags that indicates "this tag is just a string, not a key-value pair". How this sentinel will be used:
--select tag:foois provided, we'll interpret it asAssetSelection.tag("foo", "__dagster_novalue")(implemented in this PR){"foo": "fooval", "bar": "__dagster_novalue"}will be rendered as "foo=fooval, bar" in the UI.How I Tested These Changes