Uh oh!
There was an error while loading. Please reload this page.
Spawner: Confirm startup was successful - #810
Conversation
WalkthroughBootup methods now return a boolean across interface and spawner components. SocketInterface.bootup returns the underlying spawner’s status. SubprocessSpawner.bootup and FluxPythonSpawner.bootup return poll() results post-start. Tests updated to assert the boolean success flag and include a new error-handling test for a failing serial command. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant I as SocketInterface
participant S as Spawner (Subprocess/Flux)
participant P as Process/Job
T->>I: bootup(command_lst)
I->>S: bootup(command_lst)
S->>P: start/schedule
S->>S: poll() running?
S-->>I: bool (running status)
I-->>T: bool (propagated)
rect rgba(230,255,230,0.5)
note right of T: Caller can immediately know if process/job is running
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches
🧪 Generate unit tests
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #810 +/- ##
=======================================
Coverage 97.76% 97.77% =======================================
Files 32 32 Lines 1479 1481 +2 =======================================
+ Hits 1446 1448 +2
Misses 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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)
executorlib/standalone/interactive/spawner.py (1)
87-105: Make bootup success non-racy and type it as boolReturning self.poll() immediately after Popen can intermittently be False for very short-lived commands, making startup detection flaky. Treat a successful Popen as “startup succeeded” and annotate the return type.
- def bootup(+ def bootup( self, command_lst: list[str], - ):+ ) -> bool: @@ - return self.poll()+ # Successful process creation == successful bootup; ongoing liveness is tracked via poll().+ return TrueOutside this hunk (optional, for consistency across the hierarchy), consider updating the abstract method to advertise the contract:
# In BaseSpawner@abstractmethoddefbootup(self, command_lst: list[str]) ->bool: ...
🧹 Nitpick comments (2)
executorlib/standalone/interactive/communication.py (1)
109-121: Nit: docstring type should match annotationTighten the docstring type to list[str] for consistency.
- Args:- command_lst (list): list of strings to start the client process+ Args:+ command_lst (list[str]): list of strings to start the client processtests/test_standalone_interactive_communication.py (1)
122-134: Stabilize the failing-command test and add a timeout["bash", "exit"] relies on a file named “exit” and may behave differently across systems. Use bash -c 'exit 1' and add a bounded wait to avoid potential infinite loops.
- success_flag = interface.bootup(command_lst=["bash", "exit"])+ success_flag = interface.bootup(command_lst=["bash", "-c", "exit 1"]) self.assertTrue(success_flag) - while interface._spawner.poll():- sleep(0.1)+ # Wait up to ~20s for the child to exit+ for _ in range(200):+ if not interface._spawner.poll():+ break+ sleep(0.1) self.assertFalse(interface._spawner.poll()) interface.shutdown(wait=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
executorlib/standalone/interactive/communication.py(1 hunks)executorlib/standalone/interactive/spawner.py(1 hunks)executorlib/task_scheduler/interactive/spawner_flux.py(2 hunks)tests/test_standalone_interactive_communication.py(8 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). (7)
- GitHub Check: unittest_win
- GitHub Check: notebooks_integration
- GitHub Check: unittest_slurm_mpich
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- 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)
🔇 Additional comments (2)
executorlib/standalone/interactive/communication.py (1)
109-121: LGTM: bootup now propagates spawner’s statusReturning the spawner’s bootup result from SocketInterface is the right API surface.
tests/test_standalone_interactive_communication.py (1)
39-57: LGTM: tests assert bootup’s boolean contractCapturing and asserting the bootup success flag across scenarios looks good.
Also applies to: 69-87, 99-117, 142-160
| def bootup( | ||
| self, | ||
| command_lst: list[str], | ||
| ): | ||
| ) -> bool: | ||
| """ | ||
| Boot up the client process to connect to the SocketInterface. | ||
| Args: | ||
| command_lst (list[str]): List of strings to start the client process. | ||
| Raises: | ||
| ValueError: If oversubscribing is not supported for the Flux adapter or if conda environments are not supported. | ||
| Returns: | ||
| bool: Whether the interface was successfully started. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align Flux bootup semantics to avoid race with fast failures
Same flakiness risk: a job that fails near-instantly can make poll() return False at bootup. Return True on successful submit and keep poll() for liveness.
- def bootup(+ def bootup(
self,
command_lst: list[str],
) -> bool:
@@
- return self.poll()+ # Submit succeeded → startup succeeded. Use poll() for liveness checks.+ return TrueAlso applies to: 133-134
🤖 Prompt for AI Agents
In executorlib/task_scheduler/interactive/spawner_flux.py around lines 75-90
(and likewise adjust lines 133-134), the bootup method currently treats an
immediate poll() False as startup failure which races with very fast job
failures; instead, after a successful job submission return True to indicate
boot completed and rely on poll() only for later liveness checks. Modify bootup
so it verifies submission succeeded (no exceptions and valid job handle),
returns True immediately on submit success even if poll() currently returns
False, and keep poll() used elsewhere to detect runtime termination; apply the
same semantics to the similar logic at lines 133-134.
Summary by CodeRabbit