Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump rich dependency to version 13 #512

Merged
merged 6 commits into from
Oct 22, 2024
Merged

Conversation

aplund
Copy link
Contributor

@aplund aplund commented Oct 18, 2024

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.

Copy link
Contributor

PR Reviewer Guide 🔍

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.

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Specify an upper bound for dependency versions to enhance long-term stability

Consider specifying an upper bound for the rich dependency to prevent potential
compatibility issues with future major versions.

pyproject.toml [34]

-rich = "^13.9.0"
+rich = "^13.9.0,<14.0.0"
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7

💡 Need additional feedback ? start a PR chat

**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.
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.28%. Comparing base (242c806) to head (905faa4).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #512      +/-   ##
===========================================
- Coverage    89.85%   89.28%   -0.57%     
===========================================
  Files          104       86      -18     
  Lines         7698     5956    -1742     
===========================================
- Hits          6917     5318    -1599     
+ Misses         781      638     -143     

see 96 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@aplund aplund requested a review from timmysilv October 21, 2024 20:33
@apchytr apchytr self-requested a review October 21, 2024 21:08
Copy link
Collaborator

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

scipy & sphinx bumps scare me a bit but i think it's ok 🙂

@aplund
Copy link
Contributor Author

aplund commented Oct 22, 2024

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

@timmysilv
Copy link
Collaborator

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?

@apchytr
Copy link
Collaborator

apchytr commented Oct 22, 2024

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

@aplund aplund merged commit 8905170 into XanaduAI:develop Oct 22, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants