backends.dragon V3: capture per-rank stderr for function tasks - #51
backends.dragon V3: capture per-rank stderr for function tasks#51andre-merzky wants to merge 14 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces two new execution backends: EdgeExecutionBackend, which delegates task execution to remote RADICAL Edge nodes with support for submission batching and Python version compatibility checks, and NoopExecutionBackend for performance benchmarking. It also enhances the Dragon V3 backend by implementing per-rank stdout/stderr redirection and traceback capture for function tasks. Feedback highlights several critical issues in the Dragon V3 implementation, including potential output loss, blocking I/O on the event loop, and resource leaks. Additionally, inconsistencies in the Edge backend's default configuration and interface mismatches in the No-op backend were identified.
| if raised: | ||
| task_desc["exception"] = result | ||
| task_desc["stderr"] = stderr_path if stderr_path else (tb if tb else str(result)) | ||
| captured = [] | ||
| if stderr_path and not is_exec_redirect: | ||
| for f in sorted(glob.glob(f"{stderr_path}.*")): | ||
| try: | ||
| with open(f) as fh: | ||
| content = fh.read() | ||
| except Exception: | ||
| continue | ||
| if content: | ||
| rank = f.rsplit(".", 1)[-1] | ||
| captured.append(f"[rank {rank}]\n{content}") | ||
| diagnostic = "\n".join(captured) if captured else "" | ||
| if diagnostic: | ||
| try: | ||
| cls = type(result) | ||
| augmented = cls(f"{result}\n--- worker output ---\n{diagnostic}") | ||
| except Exception: | ||
| augmented = RuntimeError( | ||
| f"{result}\n--- worker output ---\n{diagnostic}" | ||
| ) | ||
| task_desc["exception"] = augmented | ||
| else: | ||
| task_desc["exception"] = result | ||
| if stderr_path and is_exec_redirect: | ||
| task_desc["stderr"] = stderr_path | ||
| else: | ||
| task_desc["stderr"] = diagnostic or str(result) | ||
| task_desc["stdout"] = ( | ||
| stdout_path if stdout_path else (stdout or task_desc.get("stdout")) | ||
| stdout_path if stdout_path and is_exec_redirect | ||
| else (stdout or task_desc.get("stdout")) | ||
| ) | ||
| self._callback_func(task_desc, "FAILED") | ||
| else: | ||
| task_desc["return_value"] = result | ||
| # Function-wrap paths are glob prefixes, not literal files — | ||
| # only point stdout/stderr at the path for exec redirect. | ||
| task_desc["stdout"] = ( | ||
| stdout_path if stdout_path else (stdout or task_desc.get("stdout")) | ||
| stdout_path if stdout_path and is_exec_redirect | ||
| else (stdout or task_desc.get("stdout")) | ||
| ) | ||
| task_desc["stderr"] = ( | ||
| stderr_path if stderr_path else (stderr or task_desc.get("stderr")) | ||
| stderr_path if stderr_path and is_exec_redirect | ||
| else (stderr or task_desc.get("stderr")) | ||
| ) | ||
| self._callback_func(task_desc, "DONE") |
There was a problem hiding this comment.
The current implementation of _deliver_batch for V3 function tasks has several issues:
- Lost Output: Since
_v3_function_wrapperredirectssys.stdoutandsys.stderrto files, Dragon's default capture mechanism (thestdoutandstderrvariables at line 3266) will return empty strings. However, this method only reads thestderrfiles on failure (lines 3281-3292) and never reads thestdoutfiles. Consequently, allstdoutis lost for function tasks, and all output is lost for successful function tasks. - Blocking I/O: The use of
glob.globandfh.read()inside_deliver_batch(which runs on the event loop viacall_soon_threadsafe) will block the loop. For jobs with many ranks or large output files, this can cause significant latency or freeze the application. - Resource Leak: The per-rank output files created in the session's work directory are never deleted, which may lead to excessive disk usage over time.
Consider reading both stdout and stderr files for all function tasks and deleting them after they are processed. To avoid blocking the event loop, perform the file I/O in a separate thread (e.g., using asyncio.to_thread or moving the logic to the _monitor_loop thread).
| rank = (os.environ.get("PMI_RANK") | ||
| or os.environ.get("OMPI_COMM_WORLD_RANK") | ||
| or os.environ.get("PALS_RANKID") | ||
| or os.environ.get("SLURM_PROCID") | ||
| or f"pid{os.getpid()}") |
There was a problem hiding this comment.
The rank detection logic in the wrapper should include DRAGON_RANK. This is the standard environment variable used by Dragon to identify ranks within a process group. Including it ensures consistency with the V1 and V2 backends and provides a reliable fallback for non-MPI environments.
| rank = (os.environ.get("PMI_RANK") | |
| or os.environ.get("OMPI_COMM_WORLD_RANK") | |
| or os.environ.get("PALS_RANKID") | |
| or os.environ.get("SLURM_PROCID") | |
| or f"pid{os.getpid()}") | |
| rank = (os.environ.get("DRAGON_RANK") | |
| or os.environ.get("PMI_RANK") | |
| or os.environ.get("OMPI_COMM_WORLD_RANK") | |
| or os.environ.get("PALS_RANKID") | |
| or os.environ.get("SLURM_PROCID") | |
| or f"pid{os.getpid()}") |
| (default 0.05). Set to 0 to disable batching. | ||
| batch_limit: Max tasks per batch — triggers an immediate flush | ||
| when reached (default 1024). | ||
| notify_batch_window: Edge-side notification batch window | ||
| (seconds). ``None`` uses server default. | ||
| notify_batch_size: Edge-side notification batch size. | ||
| ``None`` uses server default. | ||
| """ | ||
|
|
||
| _DEFAULT_BATCH_WINDOW = 0.25 |
| self.logger = logging.getLogger(__name__) | ||
| self._bridge_url = bridge_url | ||
| self._edge_name = edge_name | ||
| self._remote_backends = backends or ['dragon_v3'] |
There was a problem hiding this comment.
The default remote backend is set to ['dragon_v3'], but the DragonExecutionBackendV3 class defaults its name to "dragon". Unless the remote session explicitly overrides the backend name, this default will cause a 'Backend not found' error. It is recommended to change the default to ['dragon'] or ensure it matches the standard registration name.
| StateMapper.register_backend_states_with_defaults(backend=self) | ||
| StateMapper.register_backend_tasks_states_with_defaults(backend=self) | ||
|
|
||
| self._rh = self._get_rhapsody_handle() |
There was a problem hiding this comment.
The call to _get_rhapsody_handle() is synchronous and performs network I/O. Since it is called within an async method (_async_init), it will block the event loop. This should be wrapped in asyncio.to_thread to maintain responsiveness.
| self._rh = self._get_rhapsody_handle() | |
| self._rh = await asyncio.to_thread(self._get_rhapsody_handle) |
| async def submit_tasks(self, tasks: list[dict[str, Any]]) -> list[asyncio.Task]: | ||
| if self._backend_state != BackendMainStates.RUNNING: | ||
| self._backend_state = BackendMainStates.RUNNING | ||
|
|
||
| submitted = [] | ||
| for task in tasks: | ||
| task.update({ | ||
| "return_value": True, | ||
| "stdout": "", | ||
| "stderr": "", | ||
| "exit_code": 0, | ||
| }) | ||
| self.tasks[task["uid"]] = task | ||
| future = asyncio.create_task(self._complete(task)) | ||
| submitted.append(future) | ||
| return submitted |
There was a problem hiding this comment.
The submit_tasks method signature and return type do not match the BaseBackend interface, which defines it as returning None. Maintaining interface consistency is important for backend interoperability.
| async def submit_tasks(self, tasks: list[dict[str, Any]]) -> list[asyncio.Task]: | |
| if self._backend_state != BackendMainStates.RUNNING: | |
| self._backend_state = BackendMainStates.RUNNING | |
| submitted = [] | |
| for task in tasks: | |
| task.update({ | |
| "return_value": True, | |
| "stdout": "", | |
| "stderr": "", | |
| "exit_code": 0, | |
| }) | |
| self.tasks[task["uid"]] = task | |
| future = asyncio.create_task(self._complete(task)) | |
| submitted.append(future) | |
| return submitted | |
| async def submit_tasks(self, tasks: list[dict[str, Any]]) -> None: | |
| if self._backend_state != BackendMainStates.RUNNING: | |
| self._backend_state = BackendMainStates.RUNNING | |
| for task in tasks: | |
| task.update({ | |
| "return_value": True, | |
| "stdout": "", | |
| "stderr": "", | |
| "exit_code": 0, | |
| }) | |
| self.tasks[task["uid"]] = task | |
| asyncio.create_task(self._complete(task)) |
| async def state(self) -> str: | ||
| return self._backend_state.value | ||
|
|
||
| def task_state_cb(self): |
There was a problem hiding this comment.
Resolve the automated review findings on the V3 function-stderr work and the surrounding edge/noop backends: dragon V3 (_deliver_batch / _v3_function_wrapper): - Read both per-rank stdout AND stderr files, on success and failure (previously stdout was written by the wrapper but never read back, so all function-task stdout was lost). - Move the per-rank file read+unlink off the event loop into the monitor thread via _drain_rank_files(); _deliver_batch now does no file I/O. - Unlink per-rank files after draining so they no longer leak into the work dir. - Add DRAGON_RANK to the wrapper's rank detection (the V3 backend sets it, and it aligns with V1/V2). - Restore the traceback fallback for stderr and only surface non-empty streams, fixing two _deliver_batch unit tests the PR shipped red. edge: - Wrap the blocking _get_rhapsody_handle() network I/O in asyncio.to_thread inside _async_init so it no longer blocks the loop. - Align the batch_window docstring (0.05 -> 0.25) with _DEFAULT_BATCH_WINDOW. noop: - submit_tasks now returns None per the BaseBackend interface, retaining task references to avoid GC of in-flight completions. - task_state_cb takes (task, state) per the BaseBackend interface. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the EdgeExecutionBackend for remote task execution via RADICAL Edge and the NoopExecutionBackend for benchmarking, alongside updates to the Dragon V3 backend to capture per-rank stdout/stderr and tracebacks. The review feedback highlights several critical robustness improvements, such as ensuring deserialized exceptions are instances of BaseException to prevent silent task successes, avoiding file descriptor leaks using with statements, handling non-UTF-8 characters when draining rank files, and wrapping batch flushes and shutdown sequences in try...except blocks to prevent tasks from hanging or leaking resources during failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| cls = type(result) | ||
| augmented = cls(f"{result}\n--- worker output ---\n{diagnostic}") | ||
| except Exception: | ||
| augmented = RuntimeError( | ||
| f"{result}\n--- worker output ---\n{diagnostic}" | ||
| ) |
There was a problem hiding this comment.
In _deliver_batch, cls = type(result) is used to instantiate an augmented exception. If result is not a BaseException subclass (for example, if it is a string due to serialization fallbacks across nodes), cls(...) will succeed (e.g., str(...)) but augmented will be a string. Since session.py only raises exceptions if they are instances of BaseException, a string exception will be bypassed, causing the failed task to be silently treated as successful! Adding an issubclass(cls, BaseException) check prevents this critical issue.
| try: | |
| cls = type(result) | |
| augmented = cls(f"{result}\n--- worker output ---\n{diagnostic}") | |
| except Exception: | |
| augmented = RuntimeError( | |
| f"{result}\n--- worker output ---\n{diagnostic}" | |
| ) | |
| try: | |
| cls = type(result) | |
| if not issubclass(cls, BaseException): | |
| raise TypeError("Result is not a BaseException") | |
| augmented = cls(f"{result}\n--- worker output ---\n{diagnostic}") | |
| except Exception: | |
| augmented = RuntimeError( | |
| f"{result}\n--- worker output ---\n{diagnostic}" | |
| ) |
| # Decode base64-encoded return values (bytes results) | ||
| if body.get('_return_value_encoding') == 'base64': | ||
| import base64 | ||
| body['return_value'] = base64.b64decode(body['return_value']) | ||
| del body['_return_value_encoding'] | ||
|
|
||
| # Update local task dict with remote results | ||
| for key in ('state', 'stdout', 'stderr', 'exit_code', | ||
| 'return_value', 'exception', 'traceback', 'error'): | ||
| if key in body: | ||
| task[key] = body[key] |
There was a problem hiding this comment.
In _apply_task_update, if the task failed (state == 'FAILED') or if exception/error is present in body, the exception might be a string or dict because it was serialized over the network. If it is not a BaseException instance, session.py will not recognize it as an exception and will silently treat the failed task as successful! Converting string/dict exceptions to RuntimeError ensures failures are correctly propagated to the client.
| # Decode base64-encoded return values (bytes results) | |
| if body.get('_return_value_encoding') == 'base64': | |
| import base64 | |
| body['return_value'] = base64.b64decode(body['return_value']) | |
| del body['_return_value_encoding'] | |
| # Update local task dict with remote results | |
| for key in ('state', 'stdout', 'stderr', 'exit_code', | |
| 'return_value', 'exception', 'traceback', 'error'): | |
| if key in body: | |
| task[key] = body[key] | |
| # Decode base64-encoded return values (bytes results) | |
| if body.get('_return_value_encoding') == 'base64': | |
| import base64 | |
| body['return_value'] = base64.b64decode(body['return_value']) | |
| del body['_return_value_encoding'] | |
| # Ensure exception is a BaseException if the task failed, so session.py raises it | |
| if body.get('state') == 'FAILED' or 'exception' in body or 'error' in body: | |
| exc = body.get('exception') or body.get('error') or "Unknown remote error" | |
| if not isinstance(exc, BaseException): | |
| body['exception'] = RuntimeError(str(exc)) | |
| # Update local task dict with remote results | |
| for key in ('state', 'stdout', 'stderr', 'exit_code', | |
| 'return_value', 'exception', 'traceback', 'error'): | |
| if key in body: | |
| task[key] = body[key] |
| async def _flush_batch(self): | ||
| """Send all buffered tasks in one bulk request. | ||
|
|
||
| Must be called while holding ``_batch_lock``. | ||
| """ | ||
| if not self._batch_buffer: | ||
| return | ||
|
|
||
| batch = self._batch_buffer | ||
| self._batch_buffer = [] | ||
|
|
||
| if self._flush_handle is not None: | ||
| self._flush_handle.cancel() | ||
| self._flush_handle = None | ||
|
|
||
| self._check_python_compat(batch) | ||
|
|
||
| prof = self._prof | ||
| if prof: | ||
| for t in batch: | ||
| prof.prof('task_batch_flush', uid=t.get('uid', '?')) | ||
|
|
||
| self.logger.debug("Flushing batch of %d tasks", len(batch)) | ||
| await asyncio.to_thread(self._rh.submit_tasks, batch) |
There was a problem hiding this comment.
In _flush_batch, if self._rh.submit_tasks raises an exception (e.g., due to a network error or remote service failure), the exception is unhandled because _locked_flush runs as a background task. The tasks in the batch are left in limbo and the client hangs indefinitely. Wrapping the submission in a try...except block and failing all tasks in the batch ensures that failures are correctly reported to the user.
async def _flush_batch(self):
"""Send all buffered tasks in one bulk request.
Must be called while holding ``_batch_lock``.
"""
if not self._batch_buffer:
return
batch = self._batch_buffer
self._batch_buffer = []
if self._flush_handle is not None:
self._flush_handle.cancel()
self._flush_handle = None
try:
self._check_python_compat(batch)
self.logger.debug("Flushing batch of %d tasks", len(batch))
prof = self._prof
if prof:
for t in batch:
prof.prof('task_batch_flush', uid=t.get('uid', '?'))
await asyncio.to_thread(self._rh.submit_tasks, batch)
except Exception as exc:
self.logger.error("Failed to submit batch of %d tasks: %s", len(batch), exc)
for t in batch:
uid = t.get('uid')
if uid in self._tasks:
task = self._tasks[uid]
task['exception'] = exc
task['state'] = 'FAILED'
self._callback_func(task, 'FAILED')
raise| out = open(f"{stdout_path}.{rank}", "w") | ||
| err = open(f"{stderr_path}.{rank}", "w") | ||
| old_out, old_err = sys.stdout, sys.stderr | ||
| sys.stdout, sys.stderr = out, err | ||
| try: | ||
| return user_func(*args, **kwargs) | ||
| except BaseException: | ||
| traceback.print_exc(file=err) | ||
| raise | ||
| finally: | ||
| sys.stdout, sys.stderr = old_out, old_err | ||
| out.close() | ||
| err.close() |
There was a problem hiding this comment.
In _v3_function_wrapper, if opening the second file (stderr_path) fails, the first file (stdout_path) remains open and is leaked. Using a with statement to manage both files ensures they are always closed properly and prevents resource leaks.
| out = open(f"{stdout_path}.{rank}", "w") | |
| err = open(f"{stderr_path}.{rank}", "w") | |
| old_out, old_err = sys.stdout, sys.stderr | |
| sys.stdout, sys.stderr = out, err | |
| try: | |
| return user_func(*args, **kwargs) | |
| except BaseException: | |
| traceback.print_exc(file=err) | |
| raise | |
| finally: | |
| sys.stdout, sys.stderr = old_out, old_err | |
| out.close() | |
| err.close() | |
| with open(f"{stdout_path}.{rank}", "w") as out, open(f"{stderr_path}.{rank}", "w") as err: | |
| old_out, old_err = sys.stdout, sys.stderr | |
| sys.stdout, sys.stderr = out, err | |
| try: | |
| return user_func(*args, **kwargs) | |
| except BaseException: | |
| traceback.print_exc(file=err) | |
| raise | |
| finally: | |
| sys.stdout, sys.stderr = old_out, old_err |
| for f in sorted(glob.glob(f"{path}.*")): | ||
| try: | ||
| with open(f) as fh: | ||
| content = fh.read() | ||
| except Exception: | ||
| content = "" |
There was a problem hiding this comment.
In _drain_rank_files, reading the file with the default system encoding can raise a UnicodeDecodeError if the rank's output contains non-UTF-8 binary data (e.g., from a C library or progress bar). This would cause the entire output of that rank to be discarded. Specifying encoding="utf-8" and errors="replace" makes the file reading much more robust.
| for f in sorted(glob.glob(f"{path}.*")): | |
| try: | |
| with open(f) as fh: | |
| content = fh.read() | |
| except Exception: | |
| content = "" | |
| for f in sorted(glob.glob(f"{path}.*")): | |
| try: | |
| with open(f, encoding="utf-8", errors="replace") as fh: | |
| content = fh.read() | |
| except Exception: | |
| content = "" |
| async def shutdown(self) -> None: | ||
| """Flush pending tasks, close session and BridgeClient.""" | ||
| await self._locked_flush() | ||
| self._backend_state = BackendMainStates.SHUTDOWN | ||
|
|
||
| if self._rh: | ||
| try: | ||
| self._rh.close() | ||
| except Exception as e: | ||
| self.logger.warning("Failed to close session: %s", e) | ||
| self._rh = None | ||
|
|
||
| if self._bc: | ||
| try: | ||
| self._bc.close() | ||
| except Exception as e: | ||
| self.logger.warning("Failed to close bridge client: %s", e) | ||
| self._bc = None | ||
|
|
||
| if self._prof: | ||
| self._prof.close() | ||
|
|
||
| self.logger.info("Edge execution backend shutdown complete") |
There was a problem hiding this comment.
In shutdown, if await self._locked_flush() raises an exception, the rest of the shutdown sequence is skipped, leaking the _rh and _bc clients. Wrapping it in a try...except block ensures that cleanup always runs.
async def shutdown(self) -> None:
"""Flush pending tasks, close session and BridgeClient."""
try:
await self._locked_flush()
except Exception as e:
self.logger.warning("Failed to flush pending tasks during shutdown: %s", e)
self._backend_state = BackendMainStates.SHUTDOWN
if self._rh:
try:
self._rh.close()
except Exception as e:
self.logger.warning("Failed to close session: %s", e)
self._rh = None
if self._bc:
try:
self._bc.close()
except Exception as e:
self.logger.warning("Failed to close bridge client: %s", e)
self._bc = None
if self._prof:
self._prof.close()
self.logger.info("Edge execution backend shutdown complete")Port the robustness fixes the second PR #51 review surfaced for the edge backend into orbit (the renamed successor): - _apply_task_update: coerce a non-BaseException remote exception (a string or dict serialized over the wire) to RuntimeError, so session.py raises it instead of silently treating the failed task as successful. - _flush_batch: wrap the background submit in try/except and fail the batch's tasks explicitly, so a submission error can no longer leave them hung. - shutdown: guard the pending-task flush so a flush failure can never skip the client cleanup (close session + bridge client). Add a regression test for the string-exception coercion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OrbitExecutionBackend submits tasks to a remote HPC endpoint via the ORBIT bridge/plugin infrastructure, delegating to RhapsodyClient for batching, template compression, pipelined submission and SSE-based notifications. NoopExecutionBackend is a no-op backend for performance benchmarking. Both are registered in backends.execution (orbit behind a soft import so a missing radical.orbit doesn't break the package). Reconstructed as a clean, orbit-only change on dev: the original feature/orbit branch carried unrelated resource-manager/partition work inherited from its feature/rm base, which is intentionally excluded here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BaseTask.from_dict: select the task class with .get() so keys present but set to None (e.g. prompt=None) don't misroute the task type. tox.ini: add a [testenv:unit] target for the unit suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Match feature/rm's noop.py so the backend docstring is identical on both branches, making the eventual re-merge a clean no-op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- submit_tasks / cancel_task / cancel_all_tasks: ensure the backend is initialized (idempotent _async_init) so calling them before an explicit await/async-with no longer raises AttributeError on self._rh/_bc. - _apply_task_update: ignore empty/None SSE payloads instead of crashing. - endpoint auto-selection: skip endpoints whose list_plugins() raises (offline/misbehaving) and keep probing the rest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
orbit: keep blocking network I/O off the event loop — - _async_init runs _get_rhapsody_handle via asyncio.to_thread; - _check_python_compat is now async and runs the sysinfo host_role() lookup via asyncio.to_thread (awaited from submit_tasks); - shutdown closes the rhapsody/bridge clients via asyncio.to_thread. (The _flush_batch under-lock network I/O is deferred to a follow-up.) noop: track completion futures and cancel them in cancel_task / cancel_all_tasks (firing CANCELED, guarded against double terminal callbacks); shutdown awaits cancel_all_tasks; submit_tasks returns None to match BaseBackend. Kept byte-identical to the feature/rm copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split _flush_batch into _detach_batch (swap buffer + cancel timer, under _batch_lock) and _send_batch (network send, outside _batch_lock). This lets other submitters keep buffering while a slow flush is in flight. To preserve the guarantees the batch lock previously provided: - a dedicated _send_lock serializes remote sends, so concurrent flushes neither call the (not necessarily thread-safe) client concurrently nor reorder batches; - timer-triggered flushes are tracked in _inflight_flushes and awaited by shutdown, so a batch mid-send is never dropped. Adds tests: batch lock is released during a blocked send, sends are serialized (max one in-flight), and buffered tasks are flushed on shutdown. Addresses the deferred gemini review item. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
orbit: - serialize _async_init with _init_lock (+ double-check) so concurrent first submissions can't run initialization twice now that it awaits; - retrieve the exception of a background (timer) flush in its done callback to avoid "Task exception was never retrieved" warnings; - _apply_task_update: fall back to "error" when "exception" is present but null (dict.get default only triggers on an absent key); - python-compat check also covers raw Callable functions, not only already-cloudpickle-encoded strings. noop: generate a uid when missing (setdefault) and drop tasks from the registry once terminal, so a benchmarking run of millions of tasks does not retain them all. task: route by "is not None" instead of truthiness so a present-but-falsy field (e.g. empty prompt list) is not treated as absent. Deferred: dispatching the whole SSE handler to the loop thread (bigger, riskier refactor) — left for a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_on_task_notification (invoked on the client's background SSE thread) now marshals the whole update onto the loop via call_soon_threadsafe and dispatches through _handle_notification, so every read/write of self._tasks and the task dicts happens single-threaded on the loop — never racing loop-side writers (submit / cancel / flush-error). _apply_task_update consequently fires the state callback directly. Tests: the notification tests are now async (drive one loop tick so the marshaled handler runs), plus a new test asserting an SSE update delivered from a non-loop thread is applied on the loop thread, not the caller's. Addresses the previously-deferred gemini review item. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OrbitExecutionBackend.__init__ raises ImportError when radical.orbit is absent (module-level BridgeClient is None), which broke every Tests job now that they run (CI does not install radical.orbit). The suite mocks the whole bridge/rhapsody chain anyway, so add an autouse fixture that stubs BridgeClient to a mock when the package is missing (no-op when it is installed), keeping full orbit coverage in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OrbitExecutionBackend.__init__ uses PEP 604 `str | None` parameter annotations, which Python 3.9 evaluates eagerly at definition time and rejects (unsupported operand | for type/NoneType), breaking import on the 3.9 CI job. Defer annotation evaluation so the union syntax is accepted, matching the other backend modules (radical_pilot, api.task). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
orbit: - bound memory: drop a task from _tasks (and its uid from _terminal_fired) once its terminal callback has fired; late duplicate notifications are then ignored (uid no longer tracked); - cancel_task: if the task is still buffered locally, remove it from the batch buffer and skip the remote cancel (endpoint never received it) so it is not flushed/executed after cancellation; - cancel_all_tasks: clear the batch buffer (and pending timer) so buffered tasks are not flushed after cancellation; - _apply_task_update: guard with isinstance(body, dict) so a malformed (non-dict) SSE payload cannot raise AttributeError. noop: task_state_cb(self, task, state) to match the other backends, and add the create() classmethod factory for parity. Tests updated for the prune-on-terminal semantics (final task delivered via callback), plus new coverage for pruning and cancel-while-buffered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
radical.orbit replaced its BridgeClient stack with a broker/runtime architecture (single outbound WS, topology-based discovery). Update the backend accordingly: - import EndpointRuntime instead of BridgeClient (same optional guard) - rename bridge_url -> broker_url; drop the removed notify_batch_window/ notify_batch_size session kwargs - add start_timeout / init_timeout so the whole connection + remote session wait is bounded at init, never on the submit path - discover endpoints from the local topology snapshot: skip the broker, consumers (incl. this runtime itself) and non-present entries instead of the old name-skip list and per-endpoint plugin probes - check wait_registered() explicitly (start() returns silently on timeout) and stop the runtime on failed init to not leak its threads - close the throwaway sysinfo client after the python-compat lookup so ephemeral sessions don't accumulate on the endpoint - shutdown: runtime.stop() replaces BridgeClient.close() The RhapsodyClient task surface (submit/cancel/notifications, topics, payload keys, terminal states) is unchanged; batching and notification marshaling are untouched. Tests: mocks reshaped for the runtime API, plus coverage for topology auto-selection, registration timeout, and sysinfo client cleanup. Verified live against a local broker+endpoint (auto-selected and explicit endpoint, RUNNING->DONE callbacks). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebase of the PR #51 work onto the feature/orbit base (squashed): the branch previously carried the retired feature/edge lineage; this rebuild keeps only the stderr capture itself — collect per-rank stderr for failed function tasks for diagnosis, plus the two gemini review rounds — and sheds the edge backend files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
64e86c2 to
48f9a17
Compare
Summary
_v3_function_wrapper) that redirectssys.stdout/sys.stderrto per-rank files underself._work_dirand writes the Python traceback to the stderr file before re-raising._deliver_batchglobs the per-rank files on failure and folds their contents into the augmented exception's message, preserving the original exception type so callers re-raising viatask['exception']see the real cause.capture_stdioshell-redirect path) are unaffected.Closes #50.
Why
Without this change, a function-task rank that exits non-zero leaves the awaiter with only
RuntimeError: Error(s) in user-provided code resulted in exit codes: [1] / Dragon Error Code: DRAGON_USER_CODE_ERRORand no information about why — see the issue for the full repro. The fix surfaces the actual rank-side Python traceback under a--- worker output ---section in the augmented exception's message:Implementation notes
_v3_function_wrapperis module-level (picklable) and usesfunctools.partialto bake instdout_path/stderr_path/user_funcbefore dragon's batch pickles theProcessTemplate.PMI_RANK/OMPI_COMM_WORLD_RANK/PALS_RANKID/SLURM_PROCID) and falls back topid<N>for non-MPI ProcessGroup ranks so distinct ranks land in distinct files._deliver_batchusestask_info['script_path'](set only for the executable redirect path) to distinguish glob-prefix paths (function wrap) from literal file paths (exec redirect), so executable tasks keep their existing behaviour.tb(the internal_do_task_impl → grp.join() → _manage_exceptiontrace) is intentionally not included;str(result)already carries the user-facing dragon error message.Test plan
KeyErrorfrom insideprocess_templates=[(2, {})].KeyErrortraceback for both ranks under--- worker output ---and the awaiter'sRuntimeErrorcarries the augmented message.🤖 Generated with Claude Code