Page MenuHomePhabricator

Update get_runs_with_matching_tag method on RunStorage to query for multiple tags
ClosedPublic

Authored by sashank on Tue, Oct 29, 8:41 PM.

Details

Summary

Previously, get_runs_with_matching_tag took args key and value, which lets you query for runs by one tag. This diff updates the method to take in a list of (key, value) tuples to query the RunStorage for runs that have all of the given tags.

This is a breaking change to RunStorage, but this is also the first diff in a stack to consolidate these various RunStorage methods into a single method. In the next release, we can consolidate all these breaking changes into one change that introduces the new method.

Test Plan

unit

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sashank created this revision.Tue, Oct 29, 8:41 PM
sashank edited the summary of this revision. (Show Details)Tue, Oct 29, 8:43 PM
sashank updated this revision to Diff 6065.Tue, Oct 29, 8:47 PM
sashank edited the summary of this revision. (Show Details)

up

sashank updated this revision to Diff 6066.Tue, Oct 29, 8:49 PM

Add checks

I don't have much context here so everything looks legit to me (aside from the failing buildkite tests) but had a small nit that could be cool to think about! Will lean on someone else more qualified to give the approval XD

python_modules/dagster/dagster/core/storage/runs.py
57

*nit* but useful tidbit. Whenever I see short circuit boolean functions. I usually think that any/all are better at expressing the semantics.

all(r.tags.get(key) == value for key, value in tags)

sashank updated this revision to Diff 6107.Thu, Oct 31, 7:04 AM

Fix tests

sashank updated this revision to Diff 6112.Thu, Oct 31, 8:15 AM

postgres fix

is there a reason to get this landed ahead of the bigger refactor?

I need to query for runs with two schedule tags in D1331

alangenfeld accepted this revision.Mon, Nov 4, 4:57 PM

alright - this isn't really any worse

illallowit

python_modules/dagster/dagster/core/storage/runs.py
264–266

should we rather enforce that tags is at least 1 entry instead

This revision is now accepted and ready to land.Mon, Nov 4, 4:57 PM
sashank added inline comments.Mon, Nov 4, 5:16 PM
python_modules/dagster/dagster/core/storage/runs.py
264–266

I can imagine situations where the tags argument is dynamically generated on some conditions, so I think it makes sense to support empty lists

sashank updated this revision to Diff 6192.Mon, Nov 4, 9:38 PM

rebase on master

Harbormaster failed remote builds in B4976: Diff 6193!
sashank updated this revision to Diff 6258.Tue, Nov 5, 9:58 PM

re-run tests

sashank updated this revision to Diff 6264.Tue, Nov 5, 10:27 PM

actually rebase

sashank updated this revision to Diff 6270.Tue, Nov 5, 11:40 PM

fix queries

sashank updated this revision to Diff 6272.Tue, Nov 5, 11:43 PM

group by run_body

sashank updated this revision to Diff 6273.Tue, Nov 5, 11:46 PM

multiple group by

Harbormaster failed remote builds in B5045: Diff 6272!
sashank updated this revision to Diff 6275.Tue, Nov 5, 11:57 PM

name subquery

sashank updated this revision to Diff 6281.Wed, Nov 6, 12:44 AM

add postgres tests