signac
signac copied to clipboard
Implement feature to encode and retrieve signac elements by URI.
Description
This feature implements the ability to encode certain signac elements as a URI which can then be used to retrieve said element with the signac.open() function.
Minimal example:
>>> uri = project.find_jobs({'foo' 0, 'doc.bar': True}).to_uri()
>>> print(uri)
signac://localhost/Users/csadorf/my_project/api/v1/find?sp.foo=0&doc.bar=True
>>> signac.open(uri)
signac.contrib.project.JobsCursor({'project': 'project', 'filter': '{'sp.foo': 0, 'doc.bar': 'True'}'})
Related to issue #96.
Motivation and Context
This change is motivated by the desire to reliably and unambiguously encode signac elements as unique text-strings. It is much easier to copy & paste such a URI and store it somewhere, instead of awkwardly JSON-encoded filter for example.
Types of Changes
- [ ] Documentation update
- [ ] Bug fix
- [x] New feature
- [ ] Breaking change1
1The change breaks (or has the potential to break) existing functionality.
Checklist:
- [x] I am familiar with the Contributing Guidelines.
- [x] I agree with the terms of the Contributor Agreement.
- [x] My name is on the list of contributors.
- [x] 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.
Codecov Report
Merging #189 into feature/integrated-queries will decrease coverage by
1.24%. The diff coverage is37.17%.
@@ Coverage Diff @@
## feature/integrated-queries #189 +/- ##
==============================================================
- Coverage 65.03% 63.79% -1.25%
==============================================================
Files 37 38 +1
Lines 5554 5626 +72
==============================================================
- Hits 3612 3589 -23
- Misses 1942 2037 +95
| Impacted Files | Coverage Δ | |
|---|---|---|
| signac/__init__.py | 100% <100%> (ø) |
:arrow_up: |
| signac/contrib/filterparse.py | 67.59% <26.08%> (-10.06%) |
:arrow_down: |
| signac/uri.py | 30.43% <30.43%> (ø) |
|
| signac/contrib/project.py | 87.29% <48.27%> (-2.89%) |
:arrow_down: |
| signac/contrib/job.py | 85.82% <50%> (-4.65%) |
:arrow_down: |
| signac/core/json.py | 81.57% <0%> (-13.16%) |
:arrow_down: |
| signac/common/errors.py | 72.22% <0%> (-11.12%) |
:arrow_down: |
| signac/contrib/indexing.py | 73.77% <0%> (-1.48%) |
:arrow_down: |
| signac/core/jsondict.py | 93.86% <0%> (-1.23%) |
:arrow_down: |
| ... and 11 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 8487e60...e67ddd4. Read the comment docs.
Codecov Report
Merging #189 into feature/integrated-queries will increase coverage by
0.17%. The diff coverage is80.6%.
@@ Coverage Diff @@
## feature/integrated-queries #189 +/- ##
==============================================================
+ Coverage 65.1% 65.28% +0.17%
==============================================================
Files 37 38 +1
Lines 5574 5657 +83
==============================================================
+ Hits 3629 3693 +64
- Misses 1945 1964 +19
| Impacted Files | Coverage Δ | |
|---|---|---|
| signac/__main__.py | 0% <0%> (ø) |
:arrow_up: |
| signac/contrib/import_export.py | 84.93% <100%> (ø) |
:arrow_up: |
| signac/contrib/schema.py | 89.16% <100%> (ø) |
:arrow_up: |
| signac/contrib/linked_view.py | 85.71% <100%> (ø) |
:arrow_up: |
| signac/__init__.py | 100% <100%> (ø) |
:arrow_up: |
| signac/contrib/job.py | 90.15% <50%> (-0.32%) |
:arrow_down: |
| signac/uri.py | 78.26% <78.26%> (ø) |
|
| signac/contrib/project.py | 89.91% <84.09%> (-0.18%) |
:arrow_down: |
| signac/contrib/filterparse.py | 84.8% <89.87%> (+3.35%) |
:arrow_up: |
| ... and 3 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 99a5847...8750e88. Read the comment docs.
@bdice Is this something that we can integrate with dashboard?
@bdice Is this something that we can integrate with dashboard?
Probably, but I'm not sure if it even makes sense if the URIs require absolute paths. Is there a "relative path" variant of this planned? Like a Project.open(...)?
@bdice Is this something that we can integrate with dashboard?
Probably, but I'm not sure if it even makes sense if the URIs require absolute paths. Is there a "relative path" variant of this planned? Like a
Project.open(...)?
Yes, Project.open(...) is already implemented as part of this PR.
@csadorf Can you help me understand what I should do as the assignee on this PR? It's in a "draft" form, so did you still want me to find a second reviewer? It also appears that this PR is out of sync with its target branch -- there is some stuff about signac command history showing in the diff that seems unrelated to the title of this PR.
@csadorf Can you help me understand what I should do as the assignee on this PR? It's in a "draft" form, so did you still want me to find a second reviewer? It also appears that this PR is out of sync with its target branch -- there is some stuff about signac command history showing in the diff that seems unrelated to the title of this PR.
No, I'd say at the draft stage you should just make sure that there is some kind of movement with the PR and help the creator (in this case me), move the PR to the point that is "ready for review". Or close it if it completely stalls out, i.e., the creator doesn't respond anymore and you can't find anyone to take over.
Or close it if it completely stalls out [...]
@csadorf What are appropriate next steps for this draft PR? I've dropped it from the v1.3 release scheduled for next week. Should it be closed for now?
Please add to 1.4 milestone and leave it open, thx.
I think this will be a really great feature, I've added it to the milestone for v1.5
We should revisit this PR after ~#188~ #332 is merged, it is pretty close to complete. As discussed with @csadorf, it might be beneficial to get this reviewed by someone more experienced with working with RESTful APIs (i.e. a web developer) to make sure that the URIs we're proposing make sense. @bdice @mikemhenry in any case we should plan to discuss plans for this PR at the dev meeting on Monday.
Brief note: We should probably directly support IRIs as a superset of URIs.
@vyasr This is no longer blocked as #332 is merged, correct?
Correct, this is no longer blocked. However, at this point I think we should plan this as a 2.1 feature so that we can focus our immediate efforts on finishing up the changes required for 2.0. I think this is the next major feature on the docket, though.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@vyasr and @csadorf, is this still more appropriate as a 2.1 feature, or can it go into 2.0?
I don't know how much more work needs to be done here. I'd leave it up to the judgement of @vyasr .
Sorry for not getting back to this any sooner. I think this is still more appropriate for 2.1. We have enough to get done to get us to a clean state for 2.0 that we'd be better served getting our ducks in line for that and then coming back to this in 2.1.