[Feature] Log Communication overhead #671
Conversation
WalkthroughA new 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #671 +/- ##
==========================================
+ Coverage 96.82% 96.85% +0.03%
==========================================
Files 29 29
Lines 1292 1305 +13
==========================================
+ Hits 1251 1264 +13
Misses 41 41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jan-janssen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
executorlib/standalone/interactive/communication.py (2)
43-46: Consider using more efficient logging practices.The logging implementation works correctly but could be improved for better performance and consistency with Python logging best practices.
Consider using f-strings or logger formatting instead of string concatenation:
- if self._logger is not None: - self._logger.warning("Send dictionary of size: " + str(sys.getsizeof(data))) + if self._logger is not None: + self._logger.warning("Send dictionary of size: %d", sys.getsizeof(data))
55-60: Apply consistent logging formatting.Similar to the send_dict method, the logging here could benefit from more efficient formatting.
Apply the same formatting improvement:
- if self._logger is not None: - self._logger.warning( - "Received dictionary of size: " + str(sys.getsizeof(data)) - ) + if self._logger is not None: + self._logger.warning("Received dictionary of size: %d", sys.getsizeof(data))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
executorlib/executor/flux.py(8 hunks)executorlib/executor/single.py(7 hunks)executorlib/executor/slurm.py(8 hunks)executorlib/standalone/interactive/communication.py(8 hunks)executorlib/task_scheduler/interactive/shared.py(2 hunks)tests/test_singlenodeexecutor_dependencies.py(1 hunks)tests/test_standalone_interactive_communication.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
executorlib/task_scheduler/interactive/shared.py (1)
executorlib/standalone/interactive/communication.py (1)
interface_bootup(134-179)
tests/test_standalone_interactive_communication.py (10)
executorlib/standalone/serialize.py (1)
cloudpickle_register(9-28)tests/test_fluxjobexecutor.py (1)
calc(19-20)tests/test_fluxpythonspawner.py (1)
calc(23-24)tests/test_interactive_dependencies.py (1)
calc(15-16)tests/test_mpiexecspawner.py (1)
calc(28-29)tests/test_backend_interactive_serial.py (1)
calc(10-11)tests/test_singlenodeexecutor_mpi.py (1)
calc(14-15)tests/test_singlenodeexecutor_noblock.py (1)
calc(7-8)executorlib/standalone/interactive/communication.py (5)
SocketInterface(10-131)bootup(90-102)bind_to_random_port(80-88)send_and_receive_dict(66-78)shutdown(104-124)executorlib/standalone/interactive/spawner.py (1)
MpiExecSpawner(138-155)
🔇 Additional comments (11)
tests/test_singlenodeexecutor_dependencies.py (1)
344-344: LGTM! Test coverage properly updated.The addition of
'log_obj_size': Falseto the expected info dictionary correctly reflects the new parameter in theSingleNodeExecutorand maintains test coverage.executorlib/task_scheduler/interactive/shared.py (1)
26-27: LGTM! Parameter integration is well-implemented.The
log_obj_sizeparameter is properly added with:
- Appropriate default value (
False) for a debug feature- Clear documentation explaining its purpose
- Correct propagation to the
interface_bootupfunctionThis change effectively integrates the debug logging capability into the task execution flow.
Also applies to: 46-47, 54-54
executorlib/executor/single.py (1)
94-95: LGTM! Comprehensive and consistent parameter integration.The
log_obj_sizeparameter is thoroughly integrated throughout theSingleNodeExecutorimplementation:
- Added to both class constructor and factory function with proper defaults
- Well-documented in docstrings
- Correctly propagated through both dependency and non-dependency execution paths
- Properly included in
resource_dictfor downstream consumptionThe systematic approach ensures the debug feature is available regardless of the execution mode chosen.
Also applies to: 135-136, 162-163, 182-183, 195-196, 227-228, 237-238
executorlib/executor/slurm.py (1)
239-240: LGTM! Consistent implementation for interactive SLURM execution.The
log_obj_sizeparameter is properly integrated intoSlurmJobExecutor:
- Systematic addition to constructor and factory function with appropriate defaults
- Comprehensive documentation updates
- Correct propagation through both dependency execution paths
- Proper inclusion in
resource_dictNote: The parameter is intentionally not added to
SlurmClusterExecutor, which uses file-based execution that likely doesn't support communication logging.Also applies to: 275-276, 320-321, 347-348, 367-368, 380-381, 416-417, 426-427
executorlib/executor/flux.py (1)
64-65: LGTM! Consistent implementation following established patterns.The
log_obj_sizeparameter is properly integrated intoFluxJobExecutor:
- Complete parameter addition with appropriate defaults and documentation
- Systematic propagation through dependency and non-dependency execution paths
- Correct inclusion in
resource_dictfor downstream usageThe implementation follows the same pattern as other executors, where only the "Job" variants (interactive execution) receive the parameter, while "Cluster" variants (file-based execution) do not.
Also applies to: 104-105, 149-150, 180-181, 204-205, 400-401, 436-437, 451-452
tests/test_standalone_interactive_communication.py (2)
59-87: Good test structure for the new logging feature.The test renaming and expansion appropriately covers the existing functionality while explicitly setting
log_obj_size=Falseto test the disabled state. The explicit bootup/shutdown calls improve test clarity.
88-115: Well-structured test for the debug logging feature.The new test method correctly verifies that the interface functions properly with
log_obj_size=Trueenabled. The test structure mirrors the non-debug version, ensuring consistent behavior across both modes.executorlib/standalone/interactive/communication.py (4)
1-1: Appropriate import addition for the logging functionality.The
loggingimport is correctly added to support the new debug logging feature.
16-16: Well-documented parameter addition.The docstring clearly explains the purpose of the new
log_obj_sizeparameter, and the constructor signature includes an appropriate default value ofFalse.Also applies to: 19-19
29-33: Clean conditional logger initialization.The logger is appropriately created only when needed, using a reasonable logger name "executorlib". Setting
_loggertoNonewhen disabled provides a clean way to check if logging is active.
138-138: Proper parameter propagation through the interface.The
log_obj_sizeparameter is correctly added to theinterface_bootupfunction signature, documented appropriately, and properly passed to theSocketInterfaceconstructor. This ensures the feature works consistently across the entire communication stack.Also applies to: 154-154, 168-171
Example:
log warnings:
Result:
Summary by CodeRabbit