feat: Add relay support for hermes_cli adapter - #15
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…copy/pasted code Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughShared Hermes helpers are extracted into a common module and consumed by both Hermes adapters. The CLI and SDK flows now delegate configuration, relay handling, and summaries to shared code, with updated tests, fixtures, and a new relay profile. ChangesShared Hermes helper extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
…s-cli-relay Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@adapters/common/src/nemo_fabric_adapters/common/hermes.py`:
- Around line 211-245: In configure_hermes_relay, clear any previously set
HERMES_NEMO_RELAY_* environment variables before applying the new relay config
so stale ATOF/ATIF values do not leak between requests. Add the cleanup at the
start of the function, then reapply only the keys for enabled observability
sections from load_relay_plugin_config, keeping the existing behavior in
configure_hermes_relay, agent_name, and relay_model_name intact.
- Around line 302-306: relay_model_name currently reads the selected model
directly from models_payload and calls .get() on whatever is stored there, which
can crash on malformed non-dict entries. Update relay_model_name to use
selected_model_config() for resolving the chosen model config instead of
accessing the raw models map, and keep the existing fallback chain to
settings.get("model_name") and "unknown". This should be done in hermes.py
within relay_model_name, reusing selected_model_config() to safely handle bad
models.<selected> shapes.
- Around line 315-326: The artifact collection loop in hermes.py is too broad
because it uses wildcard globs for the atof/atif sections, which can attach
unrelated files from a shared directory. Update the section handling in the
artifact-collection logic so each relay section only adds the files explicitly
configured for that run, using the section’s own configuration rather than
scanning all matching *.jsonl or *.json files in the output directory. Keep the
change localized to the existing loop that builds artifacts so the filtering
remains tied to the section_name and its config.
In `@adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py`:
- Around line 46-55: The preflight API key check currently only inspects the
parent process environment, so it can reject valid configs that provide the key
through the merged subprocess environment. Update _api_key_preflight_check to
validate against the same effective environment built by build_env() (including
settings["env"] merged with os.environ) before raising the missing api_key_env
RuntimeError. Keep the check scoped to the resolved api_key_env from
settings/model_config and use the merged env as the source of truth.
In `@adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py`:
- Around line 22-28: The Hermes SDK adapter currently relies on a source-tree
sys.path hack to import nemo_fabric_adapters.common.hermes, but the package
metadata does not declare that shared adapter as a runtime dependency. Update
the Hermes SDK packaging/dependency definition so nemo_fabric_adapters.common is
included when the hermes SDK is installed, and adjust the hermes extra/runtime
requirements to pull in the shared adapter alongside hermes-agent. Use the
adapter.py import of hermes_common and the package metadata that currently has
dependencies = [] and the hermes extra as the places to update.
In `@tests/conftest.py`:
- Around line 63-65: The test setup in conftest.py is adding the local
adapters_common path to sys.path in a way that can lose import precedence to an
installed nemo_fabric_adapters package. Update the sys.path handling in the
adapters_common block to insert the local path at the front instead of appending
it, so the test suite resolves the repository’s local adapter modules first.
In `@tests/test_hermes_cli_preflight.py`:
- Around line 24-27: The test is mutating process-global os.environ directly and
leaving FAB_CI_FAKE_KEY behind between parametrized runs, so update the
preflight test to use monkeypatch in the same test scope. In the test that
checks CLI preflight behavior, replace the direct os.environ assignment and
assertion around FAB_CI_FAKE_KEY with monkeypatch.setenv and monkeypatch.delenv
so each case is isolated and later tests do not inherit leaked environment
state.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae9a5ec6-eb96-4d1f-bfbb-0432b51ad339
📒 Files selected for processing (15)
adapters/common/src/nemo_fabric_adapters/common/__init__.pyadapters/common/src/nemo_fabric_adapters/common/hermes.pyadapters/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-relay.yamlpyproject.tomltests/conftest.pytests/fixtures/hermes-cli-agent/agent.yamltests/fixtures/hermes-shim-agent/adapters/hermes-shim/src/nemo_fabric_test_adapters/hermes_shim/adapter.pytests/fixtures/hermes-shim-agent/agent.yamltests/fixtures/hermes-shim-agent/profiles/harbor-swebench-django-13741.yamltests/fixtures/hermes-shim-agent/profiles/swebench-shim.yamltests/test_adapaters_hermes_common.pytests/test_hermes_cli_fields.pytests/test_hermes_cli_preflight.py
💤 Files with no reviewable changes (5)
- tests/fixtures/hermes-shim-agent/adapters/hermes-shim/src/nemo_fabric_test_adapters/hermes_shim/adapter.py
- tests/fixtures/hermes-shim-agent/agent.yaml
- tests/fixtures/hermes-shim-agent/profiles/harbor-swebench-django-13741.yaml
- tests/fixtures/hermes-shim-agent/profiles/swebench-shim.yaml
- tests/fixtures/hermes-cli-agent/agent.yaml
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
adapters/common/src/nemo_fabric_adapters/common/hermes.pySummary by CodeRabbit
agent_profilefields in Hermes-related configs/fixtures.relayinstall extra and included PyYAML for tests.