Finish migrating read_text to executor #5698
Conversation
mdegat01
added
the
refactor
mdegat01
force-pushed
the
read-text-to-executor-cont
branch
4 times, most recently
from
dc9cade to
ef7c5da
Compare
|
Warning Rate limit exceeded@mdegat01 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe changes introduce asynchronous handling across multiple modules in the codebase. Many methods previously implemented synchronously have been updated to use Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client/Test
participant HA as HomeAssistant/Addon
participant EX as Executor
C->>HA: await write_pulse()
HA->>EX: run nested write_pulse_config asynchronously
EX-->>HA: configuration written / result returned
HA-->>C: pulse write complete
sequenceDiagram
participant Core as Supervisor Core
participant HW as Hardware Helper
participant EX as Executor
Core->>HW: await last_boot()
HW->>EX: read and parse /proc/stat asynchronously
EX-->>HW: return boot time
HW-->>Core: return last boot time
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Nitpick comments (3)
supervisor/hardware/helper.py (1)
45-48: Anticipate concurrency for first-time access
Multiple coroutines callinglast_boot()simultaneously could each trigger file reads before_last_bootgets assigned. It's minor, but consider using a small synchronization if multiple parallel calls tolast_boot()are expected.tests/addons/test_addon.py (1)
254-254: Mocking async method withpatch.object
Patching the asynclast_bootmethod with a directreturn_value=utcnow()works, but consider using anAsyncMockfor clarity when mocking coroutines.supervisor/plugins/audio.py (1)
106-107: Local functionsetup_default_asound
Defining this small function to copy the template is straightforward. Mark it carefully if you expect to extend the logic or add more checks in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
supervisor/addons/addon.py(5 hunks)supervisor/api/middleware/security.py(1 hunks)supervisor/api/utils.py(5 hunks)supervisor/arch.py(1 hunks)supervisor/core.py(2 hunks)supervisor/hardware/helper.py(3 hunks)supervisor/homeassistant/core.py(1 hunks)supervisor/homeassistant/module.py(1 hunks)supervisor/host/logs.py(1 hunks)supervisor/plugins/audio.py(5 hunks)supervisor/plugins/dns.py(8 hunks)supervisor/resolution/evaluations/apparmor.py(1 hunks)supervisor/resolution/evaluations/lxc.py(1 hunks)supervisor/utils/json.py(2 hunks)tests/addons/test_addon.py(2 hunks)tests/hardware/test_helper.py(1 hunks)tests/homeassistant/test_module.py(1 hunks)tests/utils/test_log_format.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
supervisor/arch.pysupervisor/core.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
supervisor/arch.pysupervisor/core.py
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
supervisor/arch.pysupervisor/core.py
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
supervisor/arch.pysupervisor/core.py
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
supervisor/arch.pysupervisor/core.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
supervisor/arch.pysupervisor/core.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.2
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (40)
supervisor/arch.py (1)
53-53: Excellent migration to asynchronous execution!The change from synchronous to asynchronous file I/O correctly implements the PR's goal of migrating I/O operations to the executor. This prevents blocking the main event loop when reading the JSON file.
supervisor/homeassistant/core.py (1)
345-345: Proper awaiting of the now asynchronous write_pulse methodThis change correctly adds the
awaitkeyword to call the now asynchronouswrite_pulsemethod, aligning with the PR objective of migrating I/O operations to use the executor.supervisor/resolution/evaluations/lxc.py (1)
37-47: Well-structured refactoring to use executor for I/O operationsThe implementation effectively moves file I/O operations to a separate thread by:
- Creating a helper function
check_lxcthat encapsulates the file reading logic- Using
sys_run_in_executorto run this function in a thread pool- Maintaining the same error handling with
suppressThis change aligns perfectly with the PR goal of migrating I/O operations to the executor.
supervisor/utils/json.py (2)
51-54: Documentation improvement for executor requirementThe updated docstring clearly indicates that this function must be run in an executor, which is valuable information for developers using this utility function.
73-76: Documentation improvement for executor requirementThe updated docstring clearly indicates that this function must be run in an executor, which is valuable information for developers using this utility function.
tests/hardware/test_helper.py (1)
90-96: Function correctly converted to async to match implementation changes.This test has been properly updated to use
async/awaitpattern to match the changes in thelast_boot()method. The function is now defined as asynchronous and properly awaits the result fromcoresys.hardware.helper.last_boot().supervisor/api/middleware/security.py (1)
211-213: Added await to api_return_error call to match updated async signature.This change correctly adds the
awaitkeyword to theapi_return_errorfunction call, which has been converted to an asynchronous function as part of the I/O operations migration.tests/utils/test_log_format.py (2)
6-12: Test function properly converted to async.The test function has been correctly updated to be asynchronous and properly awaits the result of
format_message()which has been converted to an async function.
15-21: Second test function properly converted to async.This test function has also been correctly updated to be asynchronous, with proper awaiting of the
format_message()call.supervisor/host/logs.py (1)
75-77: Properly migrated read_json_file to use the executor.The synchronous
read_json_filecall has been correctly migrated to run in an executor viasys_run_in_executor, which makes it non-blocking. The error handling remains intact, ensuring that configuration file errors are properly caught and logged.tests/homeassistant/test_module.py (2)
69-69: Updated test function to match async implementation.The test function has been correctly updated to be asynchronous to align with the changes in the
write_pulsemethod. This ensures that tests properly await the asynchronous implementation.
76-76: Added await calls for async operations.The calls to
coresys.homeassistant.write_pulse()are now correctly awaited, which is necessary since this method has been updated to be asynchronous.Also applies to: 83-83
supervisor/api/utils.py (3)
66-70: Added await calls in api_process function.The error handling in the
api_processwrapper now correctly awaits the asynchronousapi_return_errorfunction, ensuring proper error handling in the async context.Also applies to: 79-79
110-119: Added await calls in api_process_raw function.The error handling in the
api_process_rawwrapper correctly awaits the asynchronousapi_return_errorfunction, maintaining consistent async behavior throughout the API utilities.
131-165: Converted api_return_error to async function.The
api_return_errorfunction has been properly converted to an asynchronous function, which is consistent with the codebase's migration toward non-blocking I/O operations. The function now correctly awaits theformat_messagecall.supervisor/homeassistant/module.py (1)
316-330: File I/O operations moved to executor.The
write_pulsemethod has been appropriately converted to an async function. The synchronous file I/O operations are now properly encapsulated in a nested function and executed viasys_run_in_executor, which offloads blocking operations to a thread pool. This is an excellent improvement that prevents blocking the event loop during file operations.supervisor/hardware/helper.py (3)
28-28: Use of_last_bootto cache boot time
Storing_last_bootas a nullable datetime is a good approach to avoid re-reading/proc/staton every call oflast_boot.
51-53: Asynchronous file read in executor
Reading/proc/statviasys_run_in_executoraligns with the PR's goal of offloading I/O. This prevents blocking the event loop.
64-65: Storing last boot timestamp
Assigning_last_booton successful parsing and returning it ensures efficient subsequent accesses.supervisor/core.py (2)
226-226: Check for potential None comparison
Whenawait self.sys_hardware.helper.last_boot()returnsNone, comparing againstself.sys_config.last_bootis valid only ifself.sys_config.last_bootmight also beNone. Ensure no unintended issues occur if one side is non-None.
365-365: Async retrieval forlast_boot
Usingawait self.sys_hardware.helper.last_boot()before saving toself.sys_config.last_bootis consistent with the new async method. Confirm that savingNonehere won’t break downstream code.tests/addons/test_addon.py (2)
23-23: New import forHwHelper
ImportingHwHelperdirectly indicates the tests now leverage its asynclast_bootmethod. This is consistent with the system-wide migration.
262-264: Async offset oflast_boot
This accurately simulates a host reboot by addingboot_timedeltato the newly awaited last boot time. Iflast_boot()ever yieldsNone, an error would occur here, but your patch ensures a datetime return.supervisor/plugins/audio.py (5)
91-93: Non-blocking template file read
Usingsys_run_in_executorto read the pulse-client template is an excellent approach to avoid blocking the main loop.
111-115: Handling errors when creatingasound
CatchingOSErrorand marking the system unhealthy onEBADMSGis consistent. This ensures failures in default config creation do not silently pass.
132-132: Ensure config is written before restart
Awaiting_write_config()ensures the updated audio config is saved prior to restarting the plugin container.
141-141: Synchronized config write on start
Similarly, writing the config before starting the container keeps plugin state consistent.
186-190: Async_write_configmethod
Converting_write_configto async is aligned with the rest of the plugin. Runningwrite_json_fileinsidesys_run_in_executoroffloads I/O for better responsiveness.supervisor/plugins/dns.py (7)
154-156: Good migration to asynchronous execution!Properly moved blocking
read_textoperation to the executor usingsys_run_in_executor, maintaining error handling while making it non-blocking.
163-165: Good migration to asynchronous execution!Properly moved blocking
read_textoperation to the executor usingsys_run_in_executor, maintaining error handling while making it non-blocking.
175-176: Proper await call addedCorrectly added await for the now-asynchronous
_write_resolvmethod call.
199-200: Proper await call addedCorrectly added await for the now-asynchronous
_write_configmethod call.
208-209: Proper await call addedCorrectly added await for the now-asynchronous
_write_configmethod call.
277-310: Well-implemented method conversion to asyncGood job converting the
_write_configmethod from synchronous to asynchronous and moving the JSON file write operation to the executor. The error handling is properly maintained in the asynchronous context.
417-437: Well-implemented method conversion to asyncGood job converting the
_write_resolvmethod from synchronous to asynchronous and moving the file write operation to the executor. The error handling is properly maintained in the asynchronous context.supervisor/addons/addon.py (5)
143-143: Good simplification of initializationSimplifying the initialization of
_manual_stoptoFalsein the constructor is a clean approach, as the actual value will be set during theloadmethod.
217-219: Well-implemented async check for boot timeGood implementation of the async check for the last boot time to determine if the add-on was manually stopped.
724-725: Good migration to asynchronous executionProperly moved the blocking
write_json_fileoperation to the executor usingsys_run_in_executor, maintaining error handling while making it non-blocking.
943-967: Well-implemented method conversion to async with nested functionExcellent approach converting the
write_pulsemethod to async and using a nested functionwrite_pulse_configto encapsulate cleanup and file writing logic that's then executed throughsys_run_in_executor. This pattern makes the code cleaner and more maintainable.
1076-1076: Proper await call addedCorrectly added await for the now-asynchronous
write_pulsemethod call.
mdegat01
force-pushed
the
read-text-to-executor-cont
branch
from
ebb8f1d to
d83cd9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/addons/addon.py (1)
943-957: Propagate potential errors and handle concurrency.The nested
write_pulse_configlogic matches the pattern seen inwrite_pulsefrom other components. Similar concerns apply:
shutil.rmtree(..., ignore_errors=True)can mask I/O errors.- Concurrency considerations for multiple simultaneous calls.
If needed, add logs or checks for concurrency readiness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
supervisor/addons/addon.py(5 hunks)supervisor/api/middleware/security.py(1 hunks)supervisor/api/utils.py(5 hunks)supervisor/arch.py(1 hunks)supervisor/core.py(2 hunks)supervisor/hardware/helper.py(3 hunks)supervisor/homeassistant/core.py(1 hunks)supervisor/homeassistant/module.py(1 hunks)supervisor/host/logs.py(1 hunks)supervisor/plugins/audio.py(5 hunks)supervisor/plugins/dns.py(8 hunks)supervisor/resolution/evaluations/apparmor.py(1 hunks)supervisor/resolution/evaluations/lxc.py(1 hunks)supervisor/utils/json.py(2 hunks)tests/addons/test_addon.py(4 hunks)tests/hardware/test_helper.py(1 hunks)tests/homeassistant/test_module.py(1 hunks)tests/utils/test_log_format.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- supervisor/resolution/evaluations/lxc.py
- supervisor/utils/json.py
- supervisor/host/logs.py
- supervisor/api/middleware/security.py
- supervisor/arch.py
- supervisor/homeassistant/core.py
- supervisor/plugins/dns.py
- supervisor/api/utils.py
- supervisor/hardware/helper.py
- supervisor/core.py
- supervisor/plugins/audio.py
- supervisor/resolution/evaluations/apparmor.py
- tests/utils/test_log_format.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (13)
supervisor/homeassistant/module.py (1)
316-330: Ensure robust handling of file cleanup errors and concurrency.While this asynchronous transition is good, consider logging or handling any exceptions beyond
OSError. Usingshutil.rmtree(..., ignore_errors=True)could swallow critical errors. Also, evaluate concurrency if multiple calls towrite_pulseoccur in parallel, potentially causing race conditions when removing the existing directory.supervisor/addons/addon.py (2)
143-143: Confirm default setting of_manual_stop.Initializing
_manual_stoptoFalsealways may erase any previously persisted stop state. If this is intentional, proceed; otherwise consider preserving prior states.
217-219: Revisit_manual_stopassignment logic.The logic sets
_manual_stoptoTrueif system last boot differs from hardware last boot, which might imply a reboot or manual stop scenario. Please verify this correctly reflects the intended behavior.tests/hardware/test_helper.py (1)
90-97: Good transition to async for error checking.The asynchronous test now correctly awaits
last_boot(). This improves realism for async flows. Test coverage remains adequate.tests/homeassistant/test_module.py (3)
69-69: Function successfully converted to asyncThe test function has been properly updated to an asynchronous function to match the now-async
write_pulsemethod.
76-76: Correctly awaiting the async callThe call to
coresys.homeassistant.write_pulse()has been properly updated withawaitto handle the asynchronous execution.
83-83: Correctly awaiting the async callThe call to
coresys.homeassistant.write_pulse()has been properly updated withawaitto handle the asynchronous execution.tests/addons/test_addon.py (6)
23-23: Appropriate import for HwHelperThe import of
HwHelperis necessary for the updated implementation of thetest_watchdog_during_attachfunction.
254-254: Correctly patching the async HwHelper.last_boot methodThe test has been updated to patch the
HwHelper.last_bootmethod to return the current time. The change reflects the migration of this method to an asynchronous implementation.
262-264: Successfully updating the last_boot time with async callThe code now properly awaits the asynchronous
last_boot()method call and adds the time delta to the result. This ensures the test continues to work as expected with the now-async implementation.
741-741: Function successfully converted to asyncThe test function has been properly updated to an asynchronous function to match the now-async
write_pulsemethod.
752-752: Correctly awaiting the async callThe call to
install_addon_example.write_pulse()has been properly updated withawaitto handle the asynchronous execution.
759-759: Correctly awaiting the async callThe call to
install_addon_example.write_pulse()has been properly updated withawaitto handle the asynchronous execution.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
mdegat01
force-pushed
the
read-text-to-executor-cont
branch
from
d83cd9e to
a0a1d42
Compare
mdegat01
force-pushed
the
read-text-to-executor-cont
branch
from
a0a1d42 to
7584fd9
Compare
Proposed change
Follow up to #5688 to finish moving references to
read_textto the executor. Also covers the one call toread_bytesand some other obvious I/O in same files.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Refactor
Tests