Fix IDAKLU solver crash when running in multiple threads + when telemetry is prompted for #4583
Conversation
agriyakhetarpal
requested review from
martinjrobins,
Saransh-cpp,
kratman,
arjxn-py and
a team
as code owners
|
This approach might seem overkill but this is what came to mind first and seems like the most robust way to me, so I'd like to test this a bit more – I've converted this to a draft for now. I triggered a wheel build here, let's see if it passes: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/11808256997 I've confirmed the fix locally with a fresh PyBaMM installation (i.e., with the
across the following scenarios:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4583 +/- ##
===========================================
- Coverage 99.26% 99.26% -0.01%
===========================================
Files 302 302
Lines 22889 22866 -23
===========================================
- Hits 22721 22698 -23
Misses 168 168 ☔ View full report in Codecov by Sentry. |
|
The buttons aren't working in the notebooks, actually – I'm quite sure they were working when I was testing earlier, so I guess I made some additional changes which made them break. I'll debug later in the day, and I'll consider dropping the buttons in favour of a simple input in case I don't get them to work. |
|
I switched to a more basic implementation for Jupyter notebooks, and I've seen bug reports about how inputs don't work properly with JupyterLab <4, but I don't think (too) many people are using version 3 now, so we should be good. |
agriyakhetarpal
changed the title
|
Additionally, I tested on Google Colab by installing PyBaMM in editable mode from my branch – we should be ready to go with this. Suggestions on cleaning up the code are welcome. The only place I haven't been able to test is Windows, because I don't have a Windows machine. |
|
Linux wheel builds passing: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/11819747790/job/32930388878 |
|
Ok I will take a close look at this in an hour or so |
src/pybamm/config.py
Outdated
| if is_notebook(): # pragma: no cover | ||
| try: | ||
| from IPython.display import clear_output | ||
|
|
||
| user_input = input("Do you want to enable telemetry? (Y/n): ") | ||
| clear_output() | ||
|
|
||
| return user_input | ||
|
|
||
| except Exception: # pragma: no cover | ||
| return None |
There was a problem hiding this comment.
Not a major issue, but this has no timeout so people notebooks will just hang if they don't realize this popped up
There was a problem hiding this comment.
Yes, this is intentional. I opine that notebooks are meant to be used interactively by default – I expect this to be a one-time issue, since importing PyBaMM and choosing "yes" or "no" once would mean that the config gets saved and this never comes up again (until the config file gets deleted, of course).
| while time.time() - start_time < timeout: | ||
| rlist, _, _ = select.select([sys.stdin], [], [], 0.1) |
There was a problem hiding this comment.
If we are already using select, do we need the loop? The timeout isn't very long
| import select | ||
|
|
||
| # Save terminal settings for later | ||
| old_settings = termios.tcgetattr(sys.stdin) |
There was a problem hiding this comment.
It seems excessive that we need to change the terminal settings for this
src/pybamm/config.py
Outdated
| else: | ||
| print("Invalid input. Please enter 'Y/y' for yes or 'n/N' for no:") | ||
| user_input = get_input_or_timeout(timeout) | ||
| if user_input is None: | ||
| print("\nTimeout reached. Defaulting to not enabling telemetry.") | ||
| return False |
There was a problem hiding this comment.
I know I brought it up before, but if we just check for the yes options, we can skip having an infinite loop
There was a problem hiding this comment.
If a person presses "Enter" in haste, that enables telemetry for them (because of your suggestion that I applied above), which might not be what they want – I'm trying to reduce the chance of that happening
There was a problem hiding this comment.
So, should I keep it as "yes" if Enter is pressed or "no"? I feel the latter is the better option
Co-authored-by: Eric G. Kratz <[email protected]>
Description
This came up as an unrelated issue in #4582 when testing the wheels and apparently stems from #4441 where a thread was being used to check for the input. This caused the Linux wheel tests to fail: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/11805727556/job/32890103930. This PR redoes that with a multi-platform approach and avoids the use of threads altogether by checking for different environments: Windows via
msvcrtand Linux/macOS viatermios.A possible corner case for Jupyter Notebooks (running in VS Code versus Jupyter Notebook/JupyterLab) has also been fixed.I've resorted to a more rudimentary fix using the previous method for now, since this fix was erroneous and did not work intermittently.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: