Skip to content

Conversation

patmagee
Copy link
Contributor

Currently there is no mechanism to filter runs in WES. This has been an intentional choice to ensure we are not providing an overly burdensome API to implementors. At this point there have been several common use cases which have emerged and already implemented in the community (via TES).

This PR Implements the same filtering strategy implemented by TES as part of this PR. It does not intend to be exhaustive but give a starting point to filtering workflow runs

Copy link
Contributor

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

LGTM. My only concern is that we may not have extensive experience with the tag filtering yet. @kellrott @vsmalladi: Were there any issues with the tags?

@patmagee
Copy link
Contributor Author

@uniqueg FWIW we have implemented tag filtering at DNAstack on our Wes api and have taken a slightly different approach which I think is more natural. I just suggested the current approach since it seemed that was the way tes went, but if there is no consensus on whether it is working or not we can try something else.

Basically you can specify the tag query param as many times as you want, representing a logical and between all of the tags.

The filter has the form tag=(?<key>[\:\&]+)(:(?<value>.+))?

Which then can be used like the following

/runs?tag=foo:bar&tag=bizz&tag=sample:3320

Overall we find this pretty flexible. The use case of filtering by only tag value is really not there imo, and in practice we rarely ever see someone using just a key for filtering (although not does happen).

@uniqueg
Copy link
Contributor

uniqueg commented Sep 29, 2023

Thanks @patmagee. I like the approach you describe, but with all other things being mostly equal/equivalent, I like consistency even more :)

Less mental strain for users, devs and our future selves including similar provisions in other specs.

It's just that I never tried the TES way of implementing tag filtering myself. And so I think Kyle's and Venkat's feedback may be more valuable here.

But good to have that second option, let's see what others think.

@patmagee
Copy link
Contributor Author

@uniqueg the main problem I can see with the current approach is that there is an implicit relationship between the order of tag_key and tag_value query params.

By splitting them up you force users to think about the order that filters are supplied in, since it now matters especially when the number of tag values or keys do not match.

@patmagee
Copy link
Contributor Author

@kellrott @vsmalladi pinging you here again to see if you have any experience with how the TES filtering implementation has been received.

@patmagee patmagee changed the title Add simple run filtering Add simple run filtering on tag values Dec 11, 2023
parameters:
- name: state
description: |-
OPTIONAL. Filter rubs by state. If unspecified,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"runs" not "rubs"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks fine to me.
@kellrott @vsmalladi @vinjana : Any further comments on the RP?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the proposal is fine. I would, like @uniqueg, vote for consistency with associated standards (TES).

Maybe the current way can also be extended with backwards-compatibility? E.g., {...} could be implicit AND, [...] could be implicit OR. The only problem would be NOT, but if that is solved the syntax representing a boolean algebra is complete.

For the fun of it (ignore this, if you like) I wrote some EBNF for a JSON syntax that expresses boolean expressions in a way compatible with the current proposal (= parts of simple-and-expr below):

expr ::= and-expr | or-expr | not-expr. 

// AND
and-expr ::= simple-and-expr | full-and-expr.

simple-and-expr ::= '{' ( tag-name : tag-value | expr ), ... '}'.  // i.e. non-empty JSON dict

full-and-expr ::= '{' AND ':' expr '}'.

// OR
or-expr ::= simple-or-expr | full-or-expr.

simple-or-expr ::= '[' expr, ... ']'.    // i.e. non-empty JSON list

full-or-expr ::= '{' OR ':' simple-or-expr '}'

// NOT
not-expr ::= '{' NOT ':' expr '}'.


// Operation terminals
AND ::= "and". 

OR ::= "or".

NOT ::= "not".


// Tag terminals
tag-name ::= '"' <string> '"'.

tag-value ::= '"' <string> '"'.

Note that AND, OR, and NOT cannot be confused with tag-name "and", "or", and "not", because in the places these operational terminals occur there is always an expression following, and all expressions start with either { or [, i.e., a JSON dict or list, which are not allowed allowed for tag-names (these have to be quoted strings). So, AFAICS this is unambiguously parseable.

Encoding logical expressions in JSON has the advantage that no parser has to be written, but a normal JSON parser or validator can be used. Evaluating such a tree is also simple if recursion is used, I would then, by the standard, allow that the implementer implements a depth-limit for the expressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants