-
Notifications
You must be signed in to change notification settings - Fork 28
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
bpx is added as an optional dependency #499
bpx is added as an optional dependency #499
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Dibyendu-IITKGP, please also include a breaking changelog entry, cheers!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #499 +/- ##
========================================
Coverage 99.04% 99.04%
========================================
Files 52 52
Lines 3578 3578
========================================
Hits 3544 3544
Misses 34 34 ☔ View full report in Codecov by Sentry. |
Breaking changes would be my preference, as users now need to install with the optional |
Any imports of |
Thanks @agriyakhetarpal, how do you see this being guarded? This adds an expectation on the end user to add the optional dependency, is there an additional catch to add that provides a useful error message to users who haven't installed with the optional `bpx' dependency? |
Yes, @BradyPlanden, we'll have to document that PyBOP uses optional dependencies to handle certain operations (perhaps similar to https://docs.pybamm.org/en/latest/source/user_guide/installation/#dependencies, though it's a little tricky to manage because one forgets to update this page at times). Yes, a useful error message is also nice to have; we have some guidelines on this in place, too: https://docs.pybamm.org/en/latest/source/user_guide/contributing.html#managing-optional-dependencies-and-their-imports. It works by handling imports using a programmatic |
…' of https://github.com/pybop-team/PyBOP into 498-bug-move-bpx-to-an-optional-dependency-on-the-pybop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating this @Dibyendu-IITKGP. A few suggestions on the documentation, otherwise looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks @Dibyendu-IITKGP
Description
BPX 0.4.0 requires Pydantic<2. This causes conflict when I am trying to use Battery-Data_Toolkit.
Issue reference
Fixes #498
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:
Type of change
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ nox -s tests
$ nox -s doctest
You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks:
Thank you for contributing to our project! Your efforts help us to deliver great software.