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

Update baseline for CasADi 3.6.7 bump #4582

Merged

Conversation

agriyakhetarpal
Copy link
Member

Description

I updated https://github.com/pybamm-team/casadi-vcpkg-registry/ to build CasADi 3.6.7, and this PR updates the baseline to account for that change – so that we build our extension module against CasADi 3.6.7.

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.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ 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)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ 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:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@agriyakhetarpal
Copy link
Member Author

Triggered a build from this branch on my fork here: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/11805727556. If this passes, we should be good to go. I've marked it as a draft until then.

@kratman, this should be included in the release

@@ -37,7 +37,7 @@ classifiers = [
dependencies = [
"numpy>=1.23.5,<2.0.0",
"scipy>=1.11.4",
"casadi>=3.6.6",
"casadi>=3.6.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are linking to the C++ code, shouldn't this be a hard pin at 3.6.7? I looked at this before, but seems like a good time to do it

Copy link
Member Author

@agriyakhetarpal agriyakhetarpal Nov 12, 2024

Choose a reason for hiding this comment

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

Yes, it should be. The problem is that if CasADi 3.7.X is released and we don't update it later, the mismatch will break things at runtime across all platforms (because CasADi libs are not compatible across minor versions, as we noticed in #3100). So I don't recommend doing this right now, a better time would be when we switch the build system and move away from vcpkg. Not pinning like it is currently would break just Windows, and would allow macOS and Linux builds from source to work.

The proper solution is:

  • either, pin CasADi for both build-time and runtime, for which conda-forge arguably offers a better method with {{ pin_compatible }}. I'd like to do that in a separate PR after carefully considering it because it would be a non-weak constraint for users.
  • or, not use the CasADi Python package as a build-time dependency but build it as its own extension module (like we do for SuiteSparse and SUNDIALS). We will have a single source of truth for the CasADi version being used with that approach.

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.26%. Comparing base (cb50363) to head (69a70f7).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4582   +/-   ##
========================================
  Coverage    99.26%   99.26%           
========================================
  Files          302      302           
  Lines        22889    22889           
========================================
  Hits         22721    22721           
  Misses         168      168           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agriyakhetarpal
Copy link
Member Author

The Windows build and tests are passing, but I'm unsure (and haven't followed along) what's up with the Linux wheels – it looks like the solver crashes at runtime. https://github.com/agriyakhetarpal/PyBaMM/actions/runs/11805727556/job/32890103930. That failure is unrelated to this change, though.

@kratman
Copy link
Contributor

kratman commented Nov 12, 2024

The Windows build and tests are passing, but I'm unsure (and haven't followed along) what's up with the Linux wheels – it looks like the solver crashes at runtime. https://github.com/agriyakhetarpal/PyBaMM/actions/runs/11805727556/job/32890103930. That failure is unrelated to this change, though.

I triggered the same changes on my pybammsolvers repo to see if I get the same failures

@agriyakhetarpal
Copy link
Member Author

Looks like it's coming from here:

while True:
if time.time() - time_start > timeout:
print("\nTimeout reached. Defaulting to not enabling telemetry.")
return False
answer = []
# Create and start input thread
input_thread = threading.Thread(target=get_input)
input_thread.daemon = True
input_thread.start()
# Wait for either timeout or input
input_thread.join(timeout)

@kratman
Copy link
Contributor

kratman commented Nov 12, 2024

Looks like it's coming from here

Are you working on that one here, or should I start working on it so that it does not block the release

@agriyakhetarpal
Copy link
Member Author

Looks like it's coming from here

Are you working on that one here, or should I start working on it so that it does not block the release

Yes, I'm looking into it as we speak and I have a fix, but I'll test it in CI first and check if it works.

@kratman
Copy link
Contributor

kratman commented Nov 13, 2024

Since the crash is unrelated, is anything left on this one?

@agriyakhetarpal
Copy link
Member Author

Nothing else, this is ready to go

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review November 13, 2024 10:58
@kratman kratman merged commit 4223be8 into pybamm-team:develop Nov 13, 2024
26 checks passed
@agriyakhetarpal agriyakhetarpal deleted the update/casadi-vcpkg-registry branch November 13, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants