DaemonClient: add wait option to increase/decrease workers - #7502
Conversation
The `increase_workers` and `decrease_workers` methods sent the circus `incr`/`decr` command without the `waiting` property. Without it, the daemon replies as soon as it acknowledged the command, which is before the workers have actually been spawned or stopped, and the response does not even contain the resulting `numprocesses`. Add a `wait` keyword to both methods that is forwarded as the `waiting` property. When set, the call returns only once the daemon acted on the command and the response contains the resulting number of workers. It defaults to `False` to keep the current behavior for existing callers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesDaemon worker operations
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DaemonClient
participant Daemon
participant Workers
DaemonClient->>Daemon: Request worker increase or decrease
Daemon->>Workers: Apply worker count change
Workers-->>Daemon: Report updated count
Daemon-->>DaemonClient: Return acknowledgement or completed count
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 #7502 +/- ##
==========================================
+ Coverage 80.67% 80.68% +0.02%
==========================================
Files 581 581
Lines 47002 47002
==========================================
+ Hits 37913 37918 +5
+ Misses 9089 9084 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/engine/daemon/test_client.py`:
- Around line 399-405: Update tests/engine/daemon/test_client.py lines 399-405
in test_change_workers_without_wait and its nested await_workers helper by
annotating started_daemon_client and target and adding matching Sphinx :param:
fields; update lines 425-426 to annotate started_daemon_client and document it
with :param:; update lines 439-441 to annotate stopped_daemon_client and method
and add both :param: fields. Keep types in annotations and use Sphinx-style
docstrings consistently.
- Line 451: Update the pytest.raises match assertion in the daemon timeout test
to escape the literal period in “Connection to the daemon timed out.”, using a
raw regex literal so the message match remains exact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0405d7e-61d5-4b3b-9c75-96efdd194ada
📒 Files selected for processing (2)
src/aiida/engine/daemon/client.pytests/engine/daemon/test_client.py
edan-bainglass
left a comment
edan-bainglass
left a comment
There was a problem hiding this comment.
Easy. LGTM! Thanks @khsrali
There was a problem hiding this comment.
Are you sure circus is doing something with the waiting parameter in the case of incr and decr? I could not find the same logic for these two commands as for start/stop/restart where it definitely uses this parameter. It seems to me that circus ignores the waiting parameter in the case of incr and decr. I admit its strange, since the keyword is mentioned in the docstring (see class IncrProc). Really don't want to add this functionality into circus, since we are about to remove it in the next release.
|
@agoscinski Yes! agreed to drop circus, but whatever you replace it with, you gotta provide that public api. And |
|
well you dont test wait=False |
|
We can still add it. As u say its an API change we can support later. I just wanted to make u aware (or verify) that wait=False seems to have the same functionality as wait=True and I would at least make the user aware that this does not work as expected |
Thanks @agoscinski. @khsrali can you confirm this? Is |
We actually do, that's
Yes, as demonstrated by the tests. |
|
You are right. I see now that you |
|
Cheers @agoscinski |
4 participants
The
increase_workersanddecrease_workersmethods sent the circusincr/decrcommand without thewaitingproperty. Without it, thedaemon replies as soon as it acknowledged the command, which is before
the workers have actually been spawned or stopped, and the response does
not even contain the resulting
numprocesses.Add a
waitkeyword to both methods that is forwarded as thewaitingproperty. When set, the call returns only once the daemon acted on the
command and the response contains the resulting number of workers. It
defaults to
Falseto keep the current behavior for existing callers.Closes #7500