Implement feature to search jobs with a sp- and doc-integrated filter. #188

csadorf · 2019-05-19T02:02:23Z

Description

This patch enables the search for jobs with a statepoint- and document-
combined filter. To specify whether a key is a statepoint- or a
document-key use prefixes 'sp.' and 'doc.' respectively. No prefix is
equivalent to the 'sp.' prefix.

This patch is backwards compatible with the exception that the index
scheme was slightly modified.

Related to issue glotzerlab/signac-dashboard#25 .

Minimal example

This patch enables queries such as this one:

>>> project.find_jobs({'foo': 0, 'doc.bar': True})
signac.contrib.project.JobsCursor({'project': 'project', 'filter': '{'sp.foo': 0, 'doc.bar': True}'})
>>> # equivalent to:
>>> project.find_jobs({'foo': 0}, {'bar': True})
signac.contrib.project.JobsCursor({'project': 'project', 'filter': '{'sp.foo': 0, 'doc.bar': True}'})

Equivalently on the command line:

$ signac find foo 0 doc.bar true

All keys without a prefix are automatically interpreted as having the sp. prefix.

Motivation and Context

It is difficult to impossible to search jobs by filter and document-filter encoded by a simple string. That makes it impossible for example to search properly using signac-dashboard. This patch enables searching with one combined filter, while searching with two filters is still possible

Types of Changes

Documentation update
Bug fix
New feature
Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

I have updated the API documentation as part of the package doc-strings.
I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
I have updated the changelog.

Tasks

Add warning about the use of sp and doc as root-level keys in documents.

codecov · 2019-05-19T02:39:45Z

Codecov Report

Merging #188 into master will decrease coverage by 0.03%.
The diff coverage is 79.62%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   65.06%   65.03%   -0.04%     
==========================================
  Files          37       37              
  Lines        5542     5554      +12     
==========================================
+ Hits         3606     3612       +6     
- Misses       1936     1942       +6

Impacted Files	Coverage Δ
signac/contrib/linked_view.py	`85.71% <100%> (ø)`	⬆️
signac/contrib/import_export.py	`84.93% <100%> (ø)`	⬆️
signac/contrib/schema.py	`89.16% <100%> (ø)`	⬆️
signac/contrib/filterparse.py	`77.64% <76.47%> (+1.45%)`	⬆️
signac/contrib/project.py	`90.18% <80%> (-0.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8a08c3...8487e60. Read the comment docs.

codecov · 2019-05-19T02:39:45Z

Codecov Report

Merging #188 into master will increase coverage by 0.06%.
The diff coverage is 85.52%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   65.03%   65.10%   +0.06%     
==========================================
  Files          37       37              
  Lines        5546     5574      +28     
==========================================
+ Hits         3607     3629      +22     
- Misses       1939     1945       +6

Impacted Files	Coverage Δ
signac/contrib/project.py	`90.09% <84.00%> (-0.54%)`	⬇️
signac/contrib/filterparse.py	`81.44% <84.78%> (+5.25%)`	⬆️
signac/contrib/import_export.py	`84.93% <100.00%> (ø)`
signac/contrib/linked_view.py	`85.71% <100.00%> (ø)`
signac/contrib/schema.py	`89.16% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbe589d...99a5847. Read the comment docs.

This patch enables the search for jobs with a statepoint- and document- combined filter. To specify whether a key is a statepoint- or a document-key use prefixes 'sp.' and 'doc.' respectively. No prefix is equivalent to the 'sp.' prefix. This patch is backwards compatible with the exception that the index scheme was slightly modified.

To canonicalize all job-find filters at the JobsCursor stage.

vyasr · 2019-05-21T14:08:52Z

I updated your note to point to what I think is the relevant signac-dashboard issue, but please correct it if that's not the one you wanted.

csadorf · 2019-05-21T14:12:24Z

I updated your note to point to what I think is the relevant signac-dashboard issue, but please correct it if that's not the one you wanted.

Thx, I've edited it slightly to say "related to" instead of "resolves", because dashboard might still need a minor update to work with this properly (at least we would need to test that).

vyasr · 2019-05-21T14:25:44Z

signac/contrib/filterparse.py

        return _with_message(q, file)
+
+
+def parse_simple(tokens):


I would prefer to keep this "private" by renaming to _parse_simple and renaming the current _parse_simple to _parse_simple_single or so. I recognize that you probably did this to support importing into other modules where it's used, but I don't think we need to bloat the API.

I'm ok with the name change, however, I was actually hoping to expose this function publicly, to make it easier to implement custom user scripts like this:

import click @click.command() @click.argument('filter', nargs=-1) def cli(filter): jobs = project.find_jobs(parse_simple(filter)) cli()

I support making this a public method / official API - if it's not public then signac-dashboard has to rely on a private method.

Got it, in that case I support making it part of the API as well. I still like renaming the _parse_simple method to indicate that it operates on one filter component at a time, but we don't have to change the parse_simple method.

vyasr · 2019-05-21T14:34:35Z

signac/contrib/filterparse.py

+        elif '.' in key and key.split('.', 1)[0] in ('sp', 'doc'):
+            yield key, value
+        elif key in ('sp', 'doc'):
+            yield key, value


What is the purpose of this branch?

On a related note, should we explicitly disallow sp and doc as statepoint/document keys in addition to dots in keys?

For these cases, we need to ensure to not add 'sp' or 'doc' as prefixes, for instance with a filter that is expressed like this: {"sp": {"foo": 0}}.

Concerning your second point, sp and doc are actually valid keys, you would need to "escape" them like this: {"sp.sp.foo": 0}. However, I agree that we should probably discourage that, possibly even with warnings, and make sure to explicitly test the correctness of the behavior in that case.

I didn't consider that using sp and doc as root-level keys in a standard query format was an option, but in hindsight that makes sense. I support discouraging the usage of those as keys to future-proof ourselves against future changes where that might cause problems, though.

vyasr · 2019-05-21T14:41:35Z

signac/contrib/project.py

@@ -1306,7 +1295,7 @@ def _read_cache(self):
            return cache

    def index(self, formats=None, depth=0,
-              skip_errors=False, include_job_document=True):
+              skip_errors=False, include_job_document=True, **kwargs):


Do we actually need to support arbitrary kwargs here?

No, that is an error. I was trying to introduce backwards compatibility here, and I think we should probably still do that in the long run, but as of right now, the kwargs are unnecessary.

csadorf

Thx for your review @vyasr!

csadorf · 2019-05-21T14:47:27Z

signac/contrib/filterparse.py

        return _with_message(q, file)
+
+
+def parse_simple(tokens):


I'm ok with the name change, however, I was actually hoping to expose this function publicly, to make it easier to implement custom user scripts like this:

import click @click.command() @click.argument('filter', nargs=-1) def cli(filter): jobs = project.find_jobs(parse_simple(filter)) cli()

csadorf · 2019-05-21T14:49:52Z

signac/contrib/filterparse.py

+        elif '.' in key and key.split('.', 1)[0] in ('sp', 'doc'):
+            yield key, value
+        elif key in ('sp', 'doc'):
+            yield key, value


For these cases, we need to ensure to not add 'sp' or 'doc' as prefixes, for instance with a filter that is expressed like this: {"sp": {"foo": 0}}.

Concerning your second point, sp and doc are actually valid keys, you would need to "escape" them like this: {"sp.sp.foo": 0}. However, I agree that we should probably discourage that, possibly even with warnings, and make sure to explicitly test the correctness of the behavior in that case.

csadorf · 2019-05-21T14:51:58Z

signac/contrib/project.py

@@ -1306,7 +1295,7 @@ def _read_cache(self):
            return cache

    def index(self, formats=None, depth=0,
-              skip_errors=False, include_job_document=True):
+              skip_errors=False, include_job_document=True, **kwargs):


No, that is an error. I was trying to introduce backwards compatibility here, and I think we should probably still do that in the long run, but as of right now, the kwargs are unnecessary.

csadorf · 2019-09-02T01:26:23Z

I'm going to close this until we work on 1.3.

vyasr · 2020-04-25T22:14:02Z

@vishav1771 please comment on here and let me know if you're OK with switching to work on this for now (regarding my comments from #309).

vishav1771 · 2020-04-26T16:10:44Z

@vyasr That's okay. 👍

vishav1771 · 2020-04-29T17:34:34Z

This is moved to #332 .

csadorf requested review from a team and mikemhenry May 19, 2019 02:11

csadorf added 5 commits May 19, 2019 16:03

A JobsCursor always uses an integrated filter.

531c07e

To canonicalize all job-find filters at the JobsCursor stage.

Extend unit tests to cover integrated and mixed filters.

93a893d

Fix bugs with respect to mixed and integrated filters.

d5a55c4

Fix bug in unit test implementation.

99a5847

csadorf force-pushed the feature/integrated-queries branch from 6df43ad to 99a5847 Compare May 19, 2019 20:03

csadorf marked this pull request as ready for review May 19, 2019 20:09

csadorf requested a review from a team as a code owner May 19, 2019 20:09

csadorf mentioned this pull request May 19, 2019

Use integrated queries for searches instead of 'doc:' separation. glotzerlab/signac-dashboard#63

Closed

vyasr requested changes May 21, 2019

View reviewed changes

csadorf commented May 21, 2019

View reviewed changes

csadorf removed the request for review from mikemhenry July 5, 2019 14:02

csadorf added the enhancement New feature or request label Jul 5, 2019

csadorf modified the milestones: v2.0.0, v1.3.0 Jul 5, 2019

csadorf closed this Sep 2, 2019

csadorf mentioned this pull request Apr 23, 2020

Unify or improve consistency between groupby and groupbydoc #328

Closed

vyasr mentioned this pull request Apr 25, 2020

Implemented the CLI with Click #309

Closed

12 tasks

vyasr reopened this Apr 25, 2020

vyasr mentioned this pull request Apr 25, 2020

Implement feature to encode and retrieve signac elements by URI. #189

Draft

12 tasks

vyasr marked this pull request as draft April 26, 2020 16:09

csadorf assigned vishav1771 Apr 26, 2020

csadorf modified the milestones: v1.3.0, v1.5.0 Apr 26, 2020

csadorf closed this May 2, 2020

vishav1771 mentioned this pull request May 7, 2020

Implement feature to search jobs with a sp- and doc-integrated filter #332

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement feature to search jobs with a sp- and doc-integrated filter. #188

Implement feature to search jobs with a sp- and doc-integrated filter. #188

csadorf commented May 19, 2019 •

edited

Loading

codecov bot commented May 19, 2019

codecov bot commented May 19, 2019 •

edited

Loading

vyasr commented May 21, 2019

csadorf commented May 21, 2019

vyasr May 21, 2019 •

edited

Loading

csadorf May 21, 2019

bdice May 21, 2019

vyasr May 23, 2019

vyasr May 21, 2019

csadorf May 21, 2019

vyasr May 23, 2019

vyasr May 21, 2019

csadorf May 21, 2019

csadorf left a comment

csadorf May 21, 2019

csadorf May 21, 2019

csadorf May 21, 2019

csadorf commented Sep 2, 2019

vyasr commented Apr 25, 2020

vishav1771 commented Apr 26, 2020

vishav1771 commented Apr 29, 2020

Implement feature to search jobs with a sp- and doc-integrated filter. #188

Implement feature to search jobs with a sp- and doc-integrated filter. #188

Conversation

csadorf commented May 19, 2019 • edited Loading

Description

Minimal example

Motivation and Context

Types of Changes

Checklist:

Tasks

codecov bot commented May 19, 2019

Codecov Report

codecov bot commented May 19, 2019 • edited Loading

Codecov Report

vyasr commented May 21, 2019

csadorf commented May 21, 2019

vyasr May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf commented Sep 2, 2019

vyasr commented Apr 25, 2020

vishav1771 commented Apr 26, 2020

vishav1771 commented Apr 29, 2020

csadorf commented May 19, 2019 •

edited

Loading

codecov bot commented May 19, 2019 •

edited

Loading

vyasr May 21, 2019 •

edited

Loading