-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
base: develop
Are you sure you want to change the base?
Remove IREE solver #4585
Conversation
I am expecting some stuff to fail and for this branch to require additional changes |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
@@ -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): |
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.
Unrelated, but this popped up while I was testing. It looked like a random network error
@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 |
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.
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.
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 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
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 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).
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:
$ 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: