i3558 improve wheel repair + add minimal IDAKLU test for built wheels #3569
Conversation
agriyakhetarpal
requested review from
martinjrobins,
kratman and
Saransh-cpp
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3569 +/- ##
===========================================
- Coverage 99.59% 99.59% -0.01%
===========================================
Files 258 258
Lines 20798 20796 -2
===========================================
- Hits 20713 20711 -2
Misses 85 85 ☔ View full report in Codecov by Sentry. |
|
Thanks, I will try to take a look at this tonight or tomorrow |
Co-authored-by: Saransh Chopra <[email protected]>
There was a problem hiding this comment.
this looks great, thanks @agriyakhetarpal. @pipliggins also had problems with the casadi rpath on mac, @pipliggins does this fix this?
|
Sadly not @martinjrobins - @agriyakhetarpal I have the same issue as that discussed in #3100. We've tracked down the issue to (potentially) being caused by me having an older version of casadi (3.6.0 rather than 3.6.4) elsewhere in my system, and idaklu is binding to that rather than the version pulled during the pybamm install. The changes you've made here don't appear to have affected this behaviour. |
|
I had the same thing in mind too very recently when trying to find a solution for #3100. I notice that the version bounds for casadi as a build-time and a run-time dependency are different (3.6.0 vs 3.6.3); it is possible and very reasonable that the IDAKLU target might have linked to an older version, say, 3.5.5 coming from our vcpkg registry when we had started supporting 3.6 which caused the serialisation error in #3100 (not sure if we had updated the version in the registry in time but that has been the only complaint we received with 23.5 so there might be more to it) Hi @pipliggins – if it's possible for you, could you create a fresh virtual environment, install casadi 3.6.4 and run the editable installation again just so it tries to link to the one inside your virtual environment? Edit: testing on my end with two instances: with and without casadi pre-installed into the venv, |
|
@agriyakhetarpal no joy. @jsbrittain previously suggested something similar - "adding 'casadi' to line 121 of noxfile.py (set_dev function): session.install("virtualenv", "cmake", "casadi")," to try and fix it via nox which also didn't seem to work. |
|
Yes, doing it via the Also, does invoking a clean |
|
I can get this from the editable install: SuiteSparse found in /Users/pybamm_user/.local/include: /Users/pybamm_user/.local/lib/libsuitesparseconfig.dylib Is there a way to isolate the IDAKLU install more to get a better trace? I've done a clean install of pybamm (deleted the entire folder and started the developer install process from scratch) for this |
I would suggest that doing Also, I think this output is truncated and starts from the part where SuiteSparse is linked, could you share it from the part where CMake tries to find |
|
This is with -vvv Building wheels for collected packages: pybamm CMake suite maintained and supported by Kitware (kitware.com/cmake). -- Found PythonInterp: /Users/pybamm_user/Documents/PyBaMM/venv_test2/bin/python3.11 (found suitable version "3.11.6", minimum required is "3.6") Options like adding '_editable___pybamm_23_9_finder.py' !! |
|
Okay, from the above logs Found python casadi path: /private/var/folders/rs/lr0lp0yn56z2bfv97q0cj_q00000gp/T/pip-build-env-2cwfxc9y/overlay/lib/python3.11/site-packages/casadiIt indeed links to the correct |
CMake showed a warning about these arguments not being used during the compilation of the project.
SuiteSparse dynamic libraries were being repeated in the list of paths without this configuration. Setting build rpaths for AMD, COLAMD, BTF, and KLU ensures that they do not reference the SuiteSparse config in the build folder but the one that is installed into the install prefix.
|
I have now gone further in this PR and fixed the RPATH via CMake for the compilation of the relevant SuiteSparse components, through which we can now get rid of |
|
The CMake parallel builds in a04fce6 have reduced the |
There was a problem hiding this comment.
@agriyakhetarpal, you may want to merge to develop to address the conflicts
agriyakhetarpal
force-pushed
the
i3558-repair-wheel
branch
2 times, most recently
from
7c82e87 to
aa87d69
Compare
agriyakhetarpal
force-pushed
the
i3558-repair-wheel
branch
from
aa87d69 to
632bcec
Compare
Description
This PR sets the runtime search path for the
idaklutarget via CMake to the location of the CasADi libraries so that they can be picked up byauditwheelanddelocateat the time of repairing the wheel. This path was strangely empty in prior versions and therefore a script was used to fix the RPATH for macOS. A small test has been added to each wheel job to check the import of the IDAKLU extension module, which, upon being failed will mark the workflow as such too.Tip
I have triggered a workflow run to build the wheels under these changes, it can be accessed from here: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/7009012889
Closes #3558
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
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)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: