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

Remove IREE solver #4585

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

kratman
Copy link
Contributor

@kratman kratman commented Nov 13, 2024

Description

Removes the IREE solver. The IREE solver only supports single precision and is difficult to update. It might be re-added in the future when it is more stable.

Fixes #4521

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

@kratman kratman self-assigned this Nov 13, 2024
@kratman
Copy link
Contributor Author

kratman commented Nov 13, 2024

I am expecting some stuff to fail and for this branch to require additional changes

@kratman kratman changed the title Remove IREE Remove IREE solver Nov 13, 2024
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.20%. Comparing base (4223be8) to head (5670fb0).
Report is 20 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4585      +/-   ##
===========================================
- Coverage    99.26%   99.20%   -0.06%     
===========================================
  Files          302      302              
  Lines        22889    22705     -184     
===========================================
- Hits         22721    22525     -196     
- Misses         168      180      +12     

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

@@ -336,7 +336,7 @@ def no_internet_connection():
conn = socket.create_connection((host, 80), 2)
conn.close()
return False
except socket.gaierror:
except (socket.gaierror, TimeoutError):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, but this popped up while I was testing. It looked like a random network error

@kratman
Copy link
Contributor Author

kratman commented Nov 13, 2024

@jsbrittain Can you look at this too please?

@@ -7,7 +7,6 @@ on:
env:
FORCE_COLOR: 3
PYBAMM_IDAKLU_EXPR_CASADI: ON
PYBAMM_IDAKLU_EXPR_IREE: ON
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please leave in the IREE solver code and just turn off this flag so it is not tested in the CI? Its going to be a lot more difficult to re-enable this in the future if the code is deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have two main concerns with disabling it this way:

  • Turning it off in the tests would cause the code to be untested, and untested code tends to decay. It would not be guaranteed to work when re-enabled
  • IREE requires a specific version of Jax to operate. This blocks us from updating the Jax version cleanly. For instance we need to update the Jax version to support 3.13, and that would mean installing a different version whenever you wanted to use IREE. This is possible, but adds a lot of extra complexity that would not be fully tested if it is disabled by default

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like untested code either, but I don't see how deleting the code from the repository makes it easier to re-enable the IREE functionality later on? If anything it will make it more difficult. I agree that the IREE code will decay if untested, but its going to decay faster if it only exists in an old commit. I think we should remove it from any documentation and slap an "experimental, internal use only and not guarenteed to work" so that no users try to use it. But the IREE backend is still our best plan for a GPU-compatible backend for PyBaMM, so I'd like to keep it so that we can (a) test any new functionality that IREE implements for double precision, and (b) see what can be achieved numerically with only single precision. If we turn if off, it won't require any additional workload to keep it maintained going forward (other than by the devs actually working on the IREE solver).

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.

Jax-IREE not working on MacOS-13
2 participants