execute_tasks_h5() wait for future to complete #712
Conversation
WalkthroughThe update modifies the shutdown logic within the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executorlib/task_scheduler/file/shared.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_old
- GitHub Check: unittest_flux_openmpi
- GitHub Check: notebooks_integration
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_win
- GitHub Check: pip_check
- GitHub Check: minimal
- GitHub Check: mypy
| while len(memory_dict) > 0: | ||
| memory_dict = { | ||
| key: _check_task_output( | ||
| task_key=key, | ||
| future_obj=value, | ||
| cache_directory=cache_dir_dict[key], | ||
| ) | ||
| for key, value in memory_dict.items() | ||
| if not value.done() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and sleep to prevent busy waiting and infinite loops.
The implementation correctly ensures all pending futures complete before shutdown, but the busy-wait loop could consume unnecessary CPU resources and potentially run indefinitely if futures never complete.
Consider adding a timeout mechanism and sleep interval to improve performance and reliability:
+import time
+
while len(memory_dict) > 0:
+ time.sleep(0.1) # Add small delay to prevent busy waiting
memory_dict = {
key: _check_task_output(
task_key=key,
future_obj=value,
cache_directory=cache_dir_dict[key],
)
for key, value in memory_dict.items()
if not value.done()
}
+ # Optional: Add timeout to prevent infinite loops
+ # if time.time() - start_time > max_wait_time:
+ # breakThis prevents excessive CPU usage while waiting for futures to complete and provides an escape mechanism for stuck futures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while len(memory_dict) > 0: | |
| memory_dict = { | |
| key: _check_task_output( | |
| task_key=key, | |
| future_obj=value, | |
| cache_directory=cache_dir_dict[key], | |
| ) | |
| for key, value in memory_dict.items() | |
| if not value.done() | |
| } | |
| import time | |
| while len(memory_dict) > 0: | |
| time.sleep(0.1) # Add small delay to prevent busy waiting | |
| memory_dict = { | |
| key: _check_task_output( | |
| task_key=key, | |
| future_obj=value, | |
| cache_directory=cache_dir_dict[key], | |
| ) | |
| for key, value in memory_dict.items() | |
| if not value.done() | |
| } | |
| # Optional: Add timeout to prevent infinite loops | |
| # if time.time() - start_time > max_wait_time: | |
| # break |
🤖 Prompt for AI Agents
In executorlib/task_scheduler/file/shared.py around lines 89 to 98, the current
loop busy-waits while checking for incomplete futures, which can cause high CPU
usage and potential infinite loops. To fix this, add a sleep interval inside the
loop to pause briefly between checks, reducing CPU consumption. Additionally,
implement a timeout mechanism that breaks the loop if futures do not complete
within a reasonable time, preventing infinite waiting. Use time tracking to
enforce the timeout and ensure the loop exits gracefully.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
=======================================
Coverage 96.89% 96.89%
=======================================
Files 29 29
Lines 1320 1322 +2
=======================================
+ Hits 1279 1281 +2
Misses 41 41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I just check out the branch and tried this on my local machine; for I would still caution that |
|
So for all my tests where I use |
|
Ok, super. On this branch and with Thanks for all the quick fixes and discussion, @jan-janssen |
Summary by CodeRabbit