[Feature] Use seperate log files for different workers #897
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughA Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/executorlib/task_scheduler/interactive/blockallocation.py (1)
110-120:⚠️ Potential issue | 🟠 MajorNew workers added via
max_workerssetter don't receive uniqueworker_idvalues.When
max_workersis increased, new threads are created without passingworker_id. This means dynamically added workers will all haveworker_id=None(or the default 0), defeating the purpose of per-worker log file separation for these workers.🔧 Suggested fix
elif self._max_workers < max_workers: new_process_lst = [ Thread( target=_execute_multiple_tasks, - kwargs=self._process_kwargs, + kwargs=self._process_kwargs + | { + "worker_id": self._max_workers + i, + "stop_function": lambda: _interrupt_bootup_dict[self._self_id], + }, ) - for _ in range(max_workers - self._max_workers) + for i in range(max_workers - self._max_workers) ]
🤖 Fix all issues with AI agents
In `@src/executorlib/task_scheduler/interactive/blockallocation.py`:
- Line 247: _spawner is being called with worker_id that can be None (from
_execute_multiple_tasks) while spawner constructors expect int; update the call
site so an int is always passed: either coerce None to a default (e.g., 0)
inside _execute_multiple_tasks when invoking
spawner(connections=spawner(cores=cores, worker_id=worker_id or 0, **kwargs)) or
change the max_workers setter to populate unique integer worker_id values into
_process_kwargs for each new worker before _execute_multiple_tasks is invoked;
touch the _execute_multiple_tasks function, the spawner(...) invocation, and the
max_workers setter/_process_kwargs population to ensure worker_id is never None.
🧹 Nitpick comments (2)
src/executorlib/standalone/interactive/spawner.py (2)
21-24: Docstring parameter order doesn't match signature.The signature has
worker_idbeforeopenmpi_oversubscribe, but the docstring lists them in reverse order. Consider reordering for consistency.📝 Suggested docstring reorder
Args: cwd (str): The current working directory. cores (int, optional): The number of cores to use. Defaults to 1. + worker_id (int): The worker ID. Defaults to 0. openmpi_oversubscribe (bool, optional): Whether to oversubscribe the cores. Defaults to False. - worker_id (int): The worker ID. Defaults to 0.
82-87: Docstring parameter order doesn't match signature.Similar to
BaseSpawner, the docstring parameter order differs from the signature. The signature hasworker_idat position 3, but the docstring lists it afterthreads_per_core.📝 Suggested docstring reorder
Args: cwd (str, optional): The current working directory. Defaults to None. cores (int, optional): The number of cores to use. Defaults to 1. + worker_id (int): The worker ID. Defaults to 0. + openmpi_oversubscribe (bool, optional): Whether to oversubscribe the cores. Defaults to False. threads_per_core (int, optional): The number of threads per core. Defaults to 1. - worker_id (int): The worker ID. Defaults to 0. - openmpi_oversubscribe (bool, optional): Whether to oversubscribe the cores. Defaults to False.
| cores=cores, | ||
| ), | ||
| connections=spawner(cores=cores, **kwargs), | ||
| connections=spawner(cores=cores, worker_id=worker_id, **kwargs), |
There was a problem hiding this comment.
Potential type mismatch: worker_id can be None but spawner expects int.
The worker_id parameter in _execute_multiple_tasks is typed as Optional[int] = None (Line 211), but all spawner constructors expect worker_id: int = 0. While Python won't raise a runtime error (the default would be used), the explicit worker_id=worker_id pass would send None to a parameter expecting int.
This will manifest when using the max_workers setter (Lines 111-116), which creates new threads without passing worker_id in _process_kwargs.
🛡️ Suggested fix
Either handle None in the spawner call:
- connections=spawner(cores=cores, worker_id=worker_id, **kwargs),
+ connections=spawner(cores=cores, worker_id=worker_id if worker_id is not None else 0, **kwargs),Or update the max_workers setter to pass unique worker_id values for new workers.
📝 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.
| connections=spawner(cores=cores, worker_id=worker_id, **kwargs), | |
| connections=spawner(cores=cores, worker_id=worker_id if worker_id is not None else 0, **kwargs), |
🤖 Prompt for AI Agents
In `@src/executorlib/task_scheduler/interactive/blockallocation.py` at line 247,
_spawner is being called with worker_id that can be None (from
_execute_multiple_tasks) while spawner constructors expect int; update the call
site so an int is always passed: either coerce None to a default (e.g., 0)
inside _execute_multiple_tasks when invoking
spawner(connections=spawner(cores=cores, worker_id=worker_id or 0, **kwargs)) or
change the max_workers setter to populate unique integer worker_id values into
_process_kwargs for each new worker before _execute_multiple_tasks is invoked;
touch the _execute_multiple_tasks function, the spawner(...) invocation, and the
max_workers setter/_process_kwargs population to ensure worker_id is never None.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/executor/test_flux_job.py (1)
113-133:⚠️ Potential issue | 🔴 CriticalTest file names are incorrect and will fail.
The implementation in
spawner_flux.pyline 127 creates log files with the formatflux_<worker_id>.out/err(with underscore:flux_0.out,flux_0.err), but the test at lines 116-117 and 136-137 checks forflux0.out/flux0.err(without underscore). The test will fail when executed because the expected file names do not match what the implementation actually creates.Update the test to expect
flux_0.outandflux_0.errinstead offlux0.outandflux0.err.
|
Feb 02, 2026 - 19:07 UTC: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #897 +/- ##
=======================================
Coverage 93.46% 93.47%
=======================================
Files 38 38
Lines 1851 1853 +2
=======================================
+ Hits 1730 1732 +2
Misses 121 121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit