Closed aplund closed 3 weeks ago
Here are some key observations to aid the review process:
β±οΈ Estimated effort to review: 1 π΅βͺβͺβͺβͺ |
π Score: 95 |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Recommended focus areas for review Dependency Update Verify that the update to Rich v13 doesn't introduce any breaking changes or compatibility issues with the rest of the project, especially in the ProgressBar class mentioned in the PR description. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Best practice |
Specify an upper bound for dependency versions to enhance long-term stability___ **Consider specifying an upper bound for therich dependency to prevent potential compatibility issues with future major versions.** [pyproject.toml [34]](https://github.com/XanaduAI/MrMustard/pull/512/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R34-R34) ```diff -rich = "^13.9.0" +rich = "^13.9.0,<14.0.0" ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
π‘ Need additional feedback ? start a PR chat
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 89.28%. Comparing base (
242c806
) to head (905faa4
). Report is 1 commits behind head on develop.
The issue with the code coverage checks seems to be changes in how docstrings and decorators are handled between versions of the "coverage" package. This changes the lock file and moves from coverage version 7.4.4 to 7.6.3. In the earlier version, docstrings and decorator executions are included, and the later version they are not. So there's a heap of stuff that appears to be less coverage, but these things aren't actual code in the codebase.
(I can't merge this without all the checks being successful, including the code coverage ones.)
i can't even find where/if this is configured. maybe there's a default value somewhere saying the max coverage delta must be <=0.15% or something like that?
@timmysilv I think it was using code coverage's default values. I've added a .yml file that we can configure to (hopefully) fix this.
Context: Old dependencies can hold back things which depend on the MrMustard, which can be very deep in the dependency tree. The
rich
package had a very old major version dependency (10..) and this could interact with other seemingly random packages when depending on MrMustard.Description of the Change: Bump the major version of
rich
to 13. The only place I can see to test this version change is in the ProgressBar class, and this seemed to work fine for me on testing using the example code in the docs for using the optimizer.Benefits: Other packages that depend on MrMustard won't get into a tricky dependency situation with a very old package.
Possible Drawbacks: Possible subtle bugs as this dependency doesn't appear to have unit tests.