Working IDAKLU standalone build and wheels #3

santacodes · 2024-09-25T13:31:00Z

Got the IDAKLU solver built and working as a separate dependency. However, the restoration of the git commits history still remains a pending task.

Installation

To build the wheels -
python -m build --wheel
To install the wheels -
python -m pip install dist/*.whl

Testing with the upstream PyBaMM repo with `pybammsolvers` as a separate dependency.

With the same venv or conda environment where pybammsolvers was installed with the procedures above, you could run the tests in PyBaMM by cloning this PyBaMM branch without IDAKLU (Note: This branch is a pure python package and builds with hatch).

Issues with the `git commit history`

I am trying to restore the history of commits for all the files that we would be moving from the PyBaMM repo to the pybammsolvers repo, the problem is mostly with git as recently we moved to the src layout in PyBaMM which basically means the files that were previously under pybamm/solvers are now under src/pybamm/solvers, implying that the files were removed from the first path and added to the second path so locally trying to filter out commits didn't work as only the commits from after the src/ layout move was getting restored, I am not sure why this happens but my best guess would be when you move a file git interprets it as the file was deleted from the original path and a new file was created in the new path. While GitHub shows the previous commit history in a separate hyperlink below the git history page of files, this doesn't happen locally when I git log -- /src/solvers. Also, I am prioritising this commit history restoration work because I think we should credit the original authors of the files. Also, we would want the git blame working on the new repository.

P.S. I have this issue posted up on StackOverflow. @kratman @agriyakhetarpal I would love to have your input on this as this is kind of the main blockage for me for the past two weeks.

kratman · 2024-09-25T13:37:04Z

If the git history starts from scratch, I will not be upset. We just have to make sure that we get the latest code from pybamm when we make the switch

kratman · 2024-09-25T14:03:34Z

@santacodes Have you looked into getting the tests ported over? That is pretty important. Way more important than syncing git history

agriyakhetarpal · 2024-09-25T14:49:31Z

I know that we have touched on this before, but only briefly – how about bringing the commit history from PyBaMM before the src/ layout migration, migrating to the src/ layout again here manually, and then bringing the commit history after the migration? I think it should be doable.

Your question on StackOverflow: https://stackoverflow.com/questions/78976790/how-do-i-restore-commit-history-for-a-specific-file-that-has-been-renamed does not really provide any details, it might be worth updating so that someone experienced with this can answer it.

As for the tests, I guess we should check out the PyBaMM repo in another folder (actions/checkout supports multiple checkouts), cd into it, and then run the tests from there. We should check out from both a release tag and the latest commit in the PyBaMM develop branch (i.e., to bring in the tip-of-tree testing I mentioned in the #separate-idaklu Slack channel), unless there is something better that I'm missing.

kratman · 2024-09-25T14:59:10Z

I know that we have touched on this before, but only briefly – how about bringing the commit history from PyBaMM before the src/ layout migration, migrating to the src/ layout again here manually, and then bringing the commit history after the migration? I think it should be doable.

Lets not make this too complicated. We can link to the PR that does the switch and not worry too much about the history. We just make sure the code is identical on the day we switch and go from there.

I don't want to see this held up due to minor issues that won't be important in a years time as the IDAKLU code keeps changing. The whole IDAKLU code went through a large refactor recently, so git blame isn't even that useful anyway.

tests/test_dummy.py

kratman

I put in a few minor comments, I am testing the build now

.gitignore

santacodes · 2024-09-25T17:28:52Z

I know that we have touched on this before, but only briefly – how about bringing the commit history from PyBaMM before the src/ layout migration, migrating to the src/ layout again here manually, and then bringing the commit history after the migration? I think it should be doable.

Lets not make this too complicated. We can link to the PR that does the switch and not worry too much about the history. We just make sure the code is identical on the day we switch and go from there.

I don't want to see this held up due to minor issues that won't be important in a years time as the IDAKLU code keeps changing. The whole IDAKLU code went through a large refactor recently, so git blame isn't even that useful anyway.

Would it be a good idea to add the original authors of the solver signed as authors/co-authors in the commit where all the files of solvers are being added? This commit specifically. I think it would be a simple thing to do and that way we wouldn't have to move all of the history of commits.

kratman · 2024-09-25T17:32:41Z

@santacodes Don't worry too much about attribution. This is still a PyBaMM team repo and their work is on PyBaMM. Just forget about that for now and lets focus on getting this working

agriyakhetarpal · 2024-09-25T18:56:47Z

Please hold on with this – I spent a bit of time towards getting the commit history thingy working and it is working as intended; I got it done pretty quickly, actually: I now have a new repository locally that contains the history for all the code under the current src/pybamm/solvers/ directory. This properly includes the commit in between where we migrated to the src layout: pybamm-team/PyBaMM@f49189c

Now that we know how to do this, I'd recommend trying this out, @santacodes:

This is what I did, you may use a similar variation:

Install git-filter-repo using pip install git-filter-repo (the source is at https://github.com/newren/git-filter-repo)
Run the following command (here are their instructions):

git filter-repo --path src/pybamm/solvers --path pybamm/solvers \
--path CMakeLists.txt \
--path pyproject.toml --path setup.py --path MANIFEST.in \
--path vcpkg-configuration.json --path vcpkg.json \
--path LICENSE.txt \
--force

i.e., to include all the files that have been relevant to the compiled code in any way over the years. You may run the command in a clone of the PyBaMM repo, and once you are done, you can modify the repo's remote to point to a different GitHub repository, and push the changes there. Please feel free to include or exclude any files in this command that I might have missed including or excluding (for example, the C++ code is only in src/pybamm/solvers/c_solvers/ and not src/pybamm/solvers/)

If there are concerns about how this will be factored into a PR, I recommend that @kratman do this in this repository, re-initialising it with the commit history (or just by creating a new repository if needed since this repository isn't being used at all right now). This is a one-time effort that should not take too long.

kratman

This has problems with its own dependencies. We need to do something about the idaklu_jax.py and idaklu_solver.py files requiring pybamm. Either that functionality needs to be migrated here as well, or they need to be removed

kratman · 2024-09-25T18:54:50Z

pyproject.toml

 description = "Python interface for the IDAKLU solver"
 requires-python = ">=3.9"
 license = {file = "LICENSE"}
 dynamic = ["version", "readme"]
-dependencies = []
+dependencies = ["pybamm", "casadi", "jax", "jaxlib"]


This library cannot depend on pybamm, otherwise it is a circular dependency

Yes, this was a hacky fix to get it working which for some reason works without a circular dependency warning, I think migrating the functionalities here might be tricky though as they are part of different parts of code and are not grouped under a single namespace.

Well here is the problem even if a circular dependency was not a major issue: PyBaMM installs IDAKLU, so how can you be certain that your install is working?

Yes, we'll need to drop PyBaMM as a dependency and adjust the relevant code, even if it means adding a few more things to the Python bindings.

Install pybamm from the branch I put up in the first PR comment, it has no IDAKLU. I made a fresh venv, installed pybamm without idaklu from the branch I mentioned and installed the pybammsolvers wheels in the same venv. That way all the tests pass. If you are still uncertain you could try running this inside a docker container and it would work, I say this because locally I don't install pip packages as system packages globally so there's no pybamm with idaklu available system-wide.

As I mentioned above, a lot of the functionalities belong to different namespaces and classes so migration wouldn't be the best way possible imo. It would be like cherry picking only the specific functionalities of code that solver uses from pybamm and including it here, I'm sure there's a better way.

I'm not sure I follow, but the plan is that we should aim for unvendoring the extension module completely – so that the pybammsolvers wheel consists of just one shared object and a Python interface to it in the same folder.

That's a great idea, but would that also mean we keep the idaklu_solver.py and idaklu_jax.py in the main PyBaMM repo and not move them here?

I think the idea is that this is only for the C++ side. The python code and dependencies should stay with pybamm whenever possible

I agree. As long as we have a reliable way of testing it with the PyBaMM test suite, we would be fine with a wheel that just contains a shared object

pyproject.toml

santacodes · 2024-09-25T19:45:30Z

Please hold on with this – I spent a bit of time towards getting the commit history thingy working and it is working as intended; I got it done pretty quickly, actually: I now have a new repository locally that contains the history for all the code under the current src/pybamm/solvers/ directory. This properly includes the commit in between where we migrated to the src layout: pybamm-team/PyBaMM@f49189c

Now that we know how to do this, I'd recommend trying this out, @santacodes:

This is what I did, you may use a similar variation:

Install git-filter-repo using pip install git-filter-repo (the source is at https://github.com/newren/git-filter-repo)

Run the following command (here are their instructions):
git filter-repo --path src/pybamm/solvers --path pybamm/solvers \
--path CMakeLists.txt \
--path pyproject.toml --path setup.py --path MANIFEST.in \
--path vcpkg-configuration.json --path vcpkg.json \
--path LICENSE.txt \
--force
i.e., to include all the files that have been relevant to the compiled code in any way over the years. You may run the command in a clone of the PyBaMM repo, and once you are done, you can modify the repo's remote to point to a different GitHub repository, and push the changes there. Please feel free to include or exclude any files in this command that I might have missed including or excluding (for example, the C++ code is only in src/pybamm/solvers/c_solvers/ and not src/pybamm/solvers/)

If there are concerns about how this will be factored into a PR, I recommend that @kratman do this in this repository, re-initialising it with the commit history (or just by creating a new repository if needed since this repository isn't being used at all right now). This is a one-time effort that should not take too long.

Thank you for the suggestion, works! 🎉

kratman · 2024-09-26T14:41:37Z

@santacodes I branched off this PR and I solved the circular dependency issues and a few other topics. I will a new PR after I finish implementing some tests. Mine is based on yours so you will still get credit in the git history

kratman · 2024-10-02T00:26:33Z

@santacodes My PR seems to be working, so I am going to close this one now

santacodes added 5 commits August 29, 2024 21:57

build commit

4949fc8

fixing build

5ecc6d4

change to has_idaklu

98083e7

final idaklu working build

55f4cb5

added uv backend to noxfile

754543b

santacodes requested a review from a team as a code owner September 25, 2024 13:31

kratman reviewed Sep 25, 2024

View reviewed changes

tests/test_dummy.py Outdated Show resolved Hide resolved

kratman requested changes Sep 25, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

refactored .gitignore

36e1d81

kratman requested changes Sep 25, 2024

View reviewed changes

remove scikit

d5fc30a

kratman closed this Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Working IDAKLU standalone build and wheels #3

Working IDAKLU standalone build and wheels #3

santacodes commented Sep 25, 2024

kratman commented Sep 25, 2024

kratman commented Sep 25, 2024

agriyakhetarpal commented Sep 25, 2024

kratman commented Sep 25, 2024

kratman left a comment

santacodes commented Sep 25, 2024

kratman commented Sep 25, 2024

agriyakhetarpal commented Sep 25, 2024

kratman left a comment

kratman Sep 25, 2024

santacodes Sep 25, 2024

kratman Sep 25, 2024

agriyakhetarpal Sep 25, 2024

santacodes Sep 25, 2024

santacodes Sep 25, 2024

agriyakhetarpal Sep 25, 2024

santacodes Sep 25, 2024

kratman Sep 25, 2024

agriyakhetarpal Sep 25, 2024

santacodes commented Sep 25, 2024

kratman commented Sep 26, 2024

kratman commented Oct 2, 2024

Working IDAKLU standalone build and wheels #3

Working IDAKLU standalone build and wheels #3

Conversation

santacodes commented Sep 25, 2024

Installation

Testing with the upstream PyBaMM repo with pybammsolvers as a separate dependency.

Issues with the git commit history

kratman commented Sep 25, 2024

kratman commented Sep 25, 2024

agriyakhetarpal commented Sep 25, 2024

kratman commented Sep 25, 2024

kratman left a comment

Choose a reason for hiding this comment

santacodes commented Sep 25, 2024

kratman commented Sep 25, 2024

agriyakhetarpal commented Sep 25, 2024

kratman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santacodes commented Sep 25, 2024

kratman commented Sep 26, 2024

kratman commented Oct 2, 2024

Testing with the upstream PyBaMM repo with `pybammsolvers` as a separate dependency.

Issues with the `git commit history`