Added JSONDecodeError for rapidjson >= 0.7.1 with fallback to ValueError.

[bdice]

Motivation and Context

Resolves #172.

Types of Changes

• Bug fix

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 changelog.

[bdice]

Before 0.7.1, the error thrown was a ValueError. This should ensure that the error in contrib.filterparse still gets caught.

[bdice]

I'd like to add a step to CI that runs tests with rapidjson if we're committed to supporting it as a primary JSON backend, going forward.

Edit: oh wait, we are testing this already. I will try to add a test that fails without this PR.

[codecov]

Codecov Report

Merging #199 into master will decrease coverage by 0.86%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
- Coverage    64.9%   64.04%   -0.87%     
==========================================
  Files          37       37              
  Lines        5557     5562       +5     
==========================================
- Hits         3607     3562      -45     
- Misses       1950     2000      +50

Impacted Files	Coverage Δ
signac/contrib/filterparse.py	76.19% <0%> (ø)	⬆️
signac/core/json.py	81.39% <83.33%> (-13.35%)	⬇️
signac/common/errors.py	72.22% <0%> (-11.12%)	⬇️
signac/contrib/job.py	86.11% <0%> (-4.37%)	⬇️
signac/contrib/indexing.py	73.77% <0%> (-1.48%)	⬇️
signac/contrib/collection.py	87.59% <0%> (-1.32%)	⬇️
signac/core/jsondict.py	93.86% <0%> (-1.23%)	⬇️
signac/core/dict_manager.py	88.37% <0%> (-1.17%)	⬇️
signac/contrib/project.py	89.58% <0%> (-1.05%)	⬇️
... and 6 more

---

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 bbf2b74...681b483. Read the comment docs.

[codecov]

Codecov Report

Merging #199 into master will increase coverage by 0.08%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage    64.9%   64.99%   +0.08%     
==========================================
  Files          37       37              
  Lines        5557     5565       +8     
==========================================
+ Hits         3607     3617      +10     
+ Misses       1950     1948       -2

Impacted Files	Coverage Δ
signac/contrib/filterparse.py	82.53% <100%> (+6.34%)	⬆️
signac/core/json.py	91.3% <77.77%> (-3.44%)	⬇️

---

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 01fc155...23cef1b. Read the comment docs.

[vyasr]

fwiw I think when we last discussed this we came down on not knowing whether rapidjson was really the right JSON backend to choose, but we needed to do a more comprehensive survey of libraries to know what to support and we hadn't assigned that to anyone.

[vyasr]

I'm not sure I follow exactly what additional test you want to add, but this PR looks good to me right now. One minor suggestion, for redirecting stderr you might want to just copy over the functions we use in testing signac-flow.

[bdice]

fwiw I think when we last discussed this we came down on not knowing whether rapidjson was really the right JSON backend to choose, but we needed to do a more comprehensive survey of libraries to know what to support and we hadn't assigned that to anyone.

@vyasr I did some reading on that subject while working on this PR. Seeing this comment on rapidjson's issue tracker suggests to me that we would be better served by ujson or simplejson or another alternative to rapidjson that provides direct support for the standard library's json API.

[bdice]

I'm not sure I follow exactly what additional test you want to add, but this PR looks good to me right now. One minor suggestion, for redirecting stderr you might want to just copy over the functions we use in testing signac-flow.

@vyasr I added the test already, so that's done. I also made the change you suggested to use signac-flow's redirection.

I think this PR is ready to merge.

[vyasr]

I agree, should be ready for merge.

And regarding choice of library, to my knowledge ujson provides the best balance of compatibility with the Python library's json module and performance, but I haven't done exhaustive research into that. We might just need to make a quick test where replace the code in signac's json module with the relevant import and see how performance improves/degrades. Actually changing which modules we support should not be too hard once we know the correct solution that provides the closest mirror to the existing functionality we expect.

[vyasr]

I just went to look at issues and found #171 which I made :D good to know we're rehashing and coming to the same conclusions, it looks like your benchmarks agree with ujson as the best alternative. @csadorf if you want to assign that issue to a particular benchmark we can assign it to someone to work on.