Skip to content

Conversation

@jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Sep 7, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Interactive tasks no longer shut down the interface when an error occurs during message exchange. Errors are now reported to the task while keeping the interface running, reducing unexpected session terminations, preserving state, and enabling smoother recovery and retries. Users should experience fewer interruptions and faster subsequent operations after transient failures, improving overall workflow stability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 7, 2025

Walkthrough

Exception handling in executorlib/task_scheduler/interactive/shared.py was updated: on failures during interface.send_and_receive_dict in both cached and non-cached execution paths, the code no longer calls interface.shutdown(wait=True). It now only records the exception on the Future. No signatures or other logic changed.

Changes

Cohort / File(s) Summary
Interactive task execution exception handling
executorlib/task_scheduler/interactive/shared.py
Removed interface.shutdown(wait=True) from exception paths in _execute_task_without_cache and _execute_task_with_cache; exceptions are now set on the Future without shutting down the interface.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant Scheduler as Task Scheduler
  participant Interface

  rect rgba(230, 240, 255, 0.6)
  note over Scheduler,Interface: Old error flow (before)
  Caller->>Scheduler: execute task
  Scheduler->>Interface: send_and_receive_dict(...)
  Interface--xScheduler: throws Exception
  Scheduler->>Interface: shutdown(wait=true)
  Scheduler->>Caller: Future.set_exception(err)
  end

  rect rgba(230, 255, 230, 0.6)
  note over Scheduler,Interface: New error flow (now)
  Caller->>Scheduler: execute task
  Scheduler->>Interface: send_and_receive_dict(...)
  Interface--xScheduler: throws Exception
  Scheduler->>Caller: Future.set_exception(err)
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • Interactive: refactor task done #795 — Adjusts the same helper functions’ exception handling to stop internal shutdowns and instead set the Future exception, mirroring this change.

Poem

I twitched my nose at errors’ sting,
No shutdown bell, no silenced ring—
Just set the fate, the Future knows,
And onward still the task stream flows.
Hop hop! Less drama, clearer trail—
A calmer warren will prevail. 🐇✨

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e05297e and 0cf681e.

📒 Files selected for processing (1)
  • executorlib/task_scheduler/interactive/shared.py (0 hunks)
💤 Files with no reviewable changes (1)
  • executorlib/task_scheduler/interactive/shared.py
⏰ 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). (5)
  • GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
  • GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
  • GitHub Check: notebooks_integration
  • GitHub Check: unittest_slurm_mpich
  • GitHub Check: unittest_win
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch keep_alive

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Sep 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.76%. Comparing base (f1847a0) to head (6fbe82e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
- Coverage   97.76%   97.76%   -0.01%     
==========================================
  Files          32       32              
  Lines        1479     1477       -2     
==========================================
- Hits         1446     1444       -2     
  Misses         33       33              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jan-janssen jan-janssen merged commit c582739 into main Sep 7, 2025
35 checks passed
@jan-janssen jan-janssen deleted the keep_alive branch September 7, 2025 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants