Rename Hermes adapter setting max_turns -> max_iterations - #14
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAcross two Hermes adapter implementations and four example profile YAML files, the iteration-limit configuration key is renamed from Changesmax_turns → max_iterations rename
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
7d85412 to
3bebb75
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py (1)
257-257: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore legacy fallback when populating
agent.max_turns(Line 257).
build_hermes_config()currently ignores legacymax_turns, so profiles still using that key won’t propagate the limit through this path. This conflicts with the backward-compat behavior implemented at Line 396.Suggested fix
- "max_turns": settings.get("max_iterations"), + "max_turns": settings.get("max_iterations", settings.get("max_turns")),🤖 Prompt for 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. In `@adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py` at line 257, The settings.get("max_iterations") call in the max_turns population logic does not include a fallback for the legacy "max_turns" key, causing profiles using the old key to be ignored in this code path. Modify the settings.get() call to first check for "max_iterations", and if it doesn't exist, fall back to checking for the legacy "max_turns" key. This will ensure consistency with the backward-compatibility behavior already implemented elsewhere in the code (as referenced in the comment regarding line 396) and allow legacy configurations to propagate correctly through the build_hermes_config() function.
🤖 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.
Duplicate comments:
In `@adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py`:
- Line 257: The settings.get("max_iterations") call in the max_turns population
logic does not include a fallback for the legacy "max_turns" key, causing
profiles using the old key to be ignored in this code path. Modify the
settings.get() call to first check for "max_iterations", and if it doesn't
exist, fall back to checking for the legacy "max_turns" key. This will ensure
consistency with the backward-compatibility behavior already implemented
elsewhere in the code (as referenced in the comment regarding line 396) and
allow legacy configurations to propagate correctly through the
build_hermes_config() function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f460e16-9b55-4db9-bdd5-54e3d563a7ac
📒 Files selected for processing (6)
adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pyexamples/code-review-agent/profiles/hermes-cli.yamlexamples/code-review-agent/profiles/hermes-relay.yamlexamples/code-review-agent/profiles/hermes-sdk.yamlexamples/code-review-agent/profiles/hermes-session.yaml
✅ Files skipped from review due to trivial changes (3)
- examples/code-review-agent/profiles/hermes-session.yaml
- examples/code-review-agent/profiles/hermes-cli.yaml
- examples/code-review-agent/profiles/hermes-relay.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/code-review-agent/profiles/hermes-sdk.yaml
- adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py
The Fabric-facing setting `max_turns` maps to Hermes' agent-loop iteration cap, not a conversational-turn count, which is misleading in session profiles where a turn is one invoke. Rename it to `max_iterations` (matching Hermes' AIAgent param) in the example profiles and read `max_iterations` in both adapters. Hermes' own config-file key (`agent.max_turns`) and the SDK `AIAgent(max_iterations=...)` parameter are unchanged, so real Hermes runs behave identically. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
3bebb75 to
01d1587
Compare
Follow-up to #12. Renames the Hermes adapter setting
max_turns→max_iterations.Why
max_turnsmaps to Hermes'AIAgent(max_iterations=…)— the cap on agent-loop iterations within a single invocation. The name reads as a conversational-turn count, which is misleading (a session "turn" is oneinvoke()).max_iterationsmatches Hermes' own parameter.What
hermes-sdk,hermes-session,hermes-cli, andhermes-relayprofiles.settings.get("max_iterations").agent.max_turns) and the SDK parameter (max_iterations) are unchanged; runtime behavior is identical.Tests
max_iterations: 1; adapter syntax + profile YAML verified. The setting only reaches Hermes on gated real-Hermes runs.Summary by CodeRabbit
max_iterationsinstead ofmax_turnsin all Hermes implementations. Users with existing Hermes configurations should update their settings to use the new parameter name.