-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Update conda environment #169
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #169 +/- ##
========================================
Coverage 96.65% 96.65%
========================================
Files 11 11
Lines 1197 1197
========================================
Hits 1157 1157
Misses 40 40 |
@TomTranter Any comments on this? |
environment.yml
Outdated
- pybamm==22.6 |
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.
Out of curiosity, is there a reason for pinning pybamm
so tightly? Adding upper caps to dependencies usually creates a lot of mess. A very good read on this - https://iscinumpy.dev/post/bound-version-constraints/
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.
I keep having issues when I update liionpack because of breaking changes introduced in PyBaMM releases. So I pinned the PyBaMM version for the conda environment to avoid this problem.
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.
IMO it should be okay if the development version of liionpack
breaks due to a breaking change in pybamm
, as that can be fixed without disturbing the users. Hard capping would make liionpack
behave as an application instead of a library.
For instance, let's say that a user has a library that depends on liionpack
. Now, if we make a new release v0.x
with pybamm==22.6
, and the user had the dependencies defined as the following -
pybamm>=22.7
liionpack
then the user will never be able to access liionpack v0.x
, as modern package managers would automatically pick up the last version of liionpack
which is compatible with pybamm>=22.7
(in this case, the version released before 0.x
).
In the long run, this would create an issue, and this is just one of the many issues associated with hard capping dependencies.
Another interesting take on this is that breaking changes are relative. One user's bug fix can be another user's breaking change. For instance, take a look at numpy/numpy#20929. The user was using a bug in NumPy
's codebase as a feature, and their project's CI went down once the bug was fixed.
But, liionpack
is not available through conda
yet (if I am not wrong), so maybe users (not developers) could be discouraged to use the conda
installation? Then we can remove this hard cap when liionpack
is released to conda
! Up to you and @TomTranter! :)
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.
There have been a few things recently with some of the pybamm and liionpack dependencies themselves requiring specific versions of other packages. sympy and protobuf in particular had version conflicts. I hope these are all sorted now and will be bound to happen from time to time. In general I don't like pinning versions and as we maintain both liionpack and pybamm we should be able to make sure they both work together on latest versions. That's my view. If we want to maintain a permanent dev branch with pinned versions so that it just works and then merge to master when things get ironed out elsewhere that could work maybe.
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.
@Saransh-cpp would it be possible to maintain a dev branch and a main branch and have dev point to pybamm dev so we can catch breaking changes early?
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.
Yes! But will this work as PyBaMM
's develop and main branch? That is pushing to main only when we are releasing a new version of liionpack
and merging the regular pull requests in develop
?
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.
If this is too much work then don't worry about it. I can make sure I manually update PyBaMM when a new version is released.
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.
I have pushed a develop
branch, but that can be abandoned if it looks like this would require extra time for maintenance. But, the workflows should be changed somehow, as we are testing liionpack
with the latest PyBaMM
commit, but we are releasing liionpack
with the latest tagged version of PyBaMM
.
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.
@wigging would you be able to apply these changes to a new PR with the develop branch as the base and remove the version pin for pybamm
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.
I changed the base branch and PyBaMM version. Had to do some rebases and force pushes so hopefully I didn't screw up anything.
This updates the conda
environment.yml
to pin the PyBaMM version and Python version. It also installs liionpack in editable mode which is useful for developing the lionpack source code in a conda environment. The README document is updated to reflect these changes.