Allow external callers to configure the hook directory - #951
Conversation
📝 WalkthroughWalkthroughAdds an optional ChangesHook directory configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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/test_cli.py`:
- Around line 49-55: The hook-dir help check only covers the dry-run command, so
it can miss regressions in other commands that should also include the shared
option. Update the test in test_hook_dir_is_available_on_run_commands to be
parameterized over the full command set covered by the common_options decorator,
including install, uninstall, run, dry-run, and generate-report, and invoke main
for each command to assert the help output still contains --hook-dir DIRECTORY.
In `@tests/test_parser.py`:
- Around line 42-47: The current test only checks Parser constructor state, so
it won’t catch regressions in path resolution inside parse(). Update
test_custom_hook_root_is_used to exercise Parser.parse() with the injected
hook_root and assert the parsed hook file resolution comes from that custom
directory, using Parser.parse and the parser.hook_root/hook_test_root setup to
verify the runtime behavior instead of just object fields.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 43241b99-55f5-4ceb-8a26-047f57f413e5
📒 Files selected for processing (5)
src/cloudai/cli/cli.pysrc/cloudai/cli/handlers.pysrc/cloudai/parser.pytests/test_cli.pytests/test_parser.py
|
we're quite busy with release + I'll be on vacations for couple weeks. I'll respond as soon as I get some spare time 🙏 |
|
@podkidyshev No worries! Take your time, happy to wait but I will update branch and merge conflicts once this is active again to avoid redundant work. |
Expose an optional --hook-dir across scenario commands and pass the resolved directory into Parser so tools such as CloudAI Autotune can invoke hook-bearing scenarios outside the CloudAI repository. Constraint: Preserve conf/hook as the parser default and keep hook-free external invocations backward compatible. Rejected: Require callers to change into the CloudAI checkout | output paths and orchestration should not depend on process working directory. Confidence: high Scope-risk: narrow Directive: Keep scenario config roots explicit at the CLI boundary when external orchestration depends on them. Tested: uv run pytest -q (1636 passed, 4 skipped); uv run pytest -q -m ci_only (490 passed); uv run pytest --dead-fixtures; uv run pre-commit run --all-files; external NCCL dry-run from CloudAI-Autotune cwd exits 0 with --hook-dir Not-tested: Live NCCL execution on a Slurm cluster. Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
Parametrize test_hook_dir_is_available_on_run_commands over all five common_options commands (install, uninstall, dry-run, run, generate-report) instead of only dry-run, so a regression on any one of them is caught. Add test_custom_hook_root_is_used_by_parse, which exercises Parser.parse() end-to-end with a custom hook_root and asserts a hook test placed only under that directory is actually resolved, rather than just checking the constructor sets hook_root/hook_test_root. Tested: python -m pytest -q (1762 passed, 18 skipped); ruff check; ruff format --check Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
5d971f4 to
e34c5b4
Compare
|
Rebased onto latest main (was 186 commits behind) to keep this mergeable — no conflicts. Also addressed both CodeRabbit review notes: |
There was a problem hiding this comment.
Pull request overview
This PR makes CloudAI’s hook discovery configurable for external orchestrators by introducing an optional --hook-dir CLI option and threading the selected hook root into Parser, while preserving the existing default behavior (conf/hook relative to the working directory) for in-repo usage.
Changes:
- Add
--hook-dirto the shared scenario CLI options forrun,dry-run,install,uninstall, andgenerate-report. - Extend
Parserto accept a configurablehook_rootand use it for hook scenario + hook test discovery. - Add regression tests covering the new CLI option and parser hook-root behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/cloudai/cli/cli.py |
Adds --hook-dir to common scenario options and passes it through command argument namespaces. |
src/cloudai/cli/handlers.py |
Instantiates Parser with the selected hook directory (or default). |
src/cloudai/parser.py |
Introduces hook_root/hook_test_root on Parser and switches hook discovery to use them. |
tests/test_cli.py |
Verifies --hook-dir appears in help output for the intended subcommands. |
tests/test_parser.py |
Adds parser regression coverage for custom hook roots and hook-test discovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cloudai/cli/handlers.py (1)
622-623: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid eagerly resolving every lazy agent while listing.
Registry.agent_names()includes unresolved entry-point names, whileRegistry.get_agent()callsep.load()and can raise when an optional agent or dependency is unavailable. One broken plugin can therefore makelist agentsfail. List names without loading classes, or catch resolution failures and display those entries as unavailable.🤖 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 `@src/cloudai/cli/handlers.py` around lines 622 - 623, Update the agent-listing loop around registry.agent_names() and registry.get_agent() so listing does not fail when a lazy or optional agent cannot be resolved. Prefer displaying names without calling get_agent; otherwise catch per-agent resolution failures and mark those entries unavailable while continuing to list the remaining agents.
🤖 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/test_parser.py`:
- Around line 49-65: Add a root-level hook scenario fixture alongside the
existing custom hook test data, then invoke Parser.parse with a non-None
scenario in test_custom_hook_root_is_used_by_parse. Assert the resulting
scenario resolution uses the custom hook root, exercising the parse_hooks branch
around the parser’s scenario-handling logic rather than only validating hook
file loading.
---
Outside diff comments:
In `@src/cloudai/cli/handlers.py`:
- Around line 622-623: Update the agent-listing loop around
registry.agent_names() and registry.get_agent() so listing does not fail when a
lazy or optional agent cannot be resolved. Prefer displaying names without
calling get_agent; otherwise catch per-agent resolution failures and mark those
entries unavailable while continuing to list the remaining agents.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: d044e709-77ae-4da0-b666-fcc5bdd2dc02
📒 Files selected for processing (5)
src/cloudai/cli/cli.pysrc/cloudai/cli/handlers.pysrc/cloudai/parser.pytests/test_cli.pytests/test_parser.py
- Remove unused HOOK_TEST_ROOT module constant (superseded by Parser.hook_test_root instance attribute). - Extend custom hook_root test coverage to also exercise scenario-level hook resolution (pre_test/post_test), not just hook test toml loading.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test_parser.py`:
- Around line 69-110: Extend
test_custom_hook_root_is_used_for_hook_scenario_resolution by adding post_test =
"custom_hook" to the main scenario fixture, then assert
test_scenario.test_runs[0].post_test is not None and its name is "custom_hook",
alongside the existing pre_test assertions.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: f6f4f75e-11f3-4bf0-945a-75e5e3b1e7a4
📒 Files selected for processing (2)
src/cloudai/parser.pytests/test_parser.py
💤 Files with no reviewable changes (1)
- src/cloudai/parser.py
|
Pushed
All green: 1777 passed, ruff/pyright clean. |
Summary
Closes #952.
--hook-dirargument to the shared scenario command options used byrun,dry-run,install,uninstall, andgenerate-report.Parserinstead of always resolving hooks fromconf/hookrelative to the process working directory.conf/hookas the default for existing in-repository workflows.CloudAI Autotune exposed this while validating CloudAI's supplied NCCL scenario from outside the CloudAI checkout. Autotune already passed absolute paths for the scenario, system config, tests, and output directory:
CloudAI still resolved hooks from
conf/hookunder Autotune's working directory and exited with:Changing the caller's working directory would make output and orchestration behavior depend on where CloudAI was launched. This change instead makes the remaining configuration root explicit, consistent with
--system-config,--tests-dir, and--test-scenario.Test Plan
Testing environment:
uvenvironmentCloudAI-Autotunerepository, not the CloudAI working directoryRegression and repository checks:
The original NCCL dry-run was then repeated from the Autotune working directory with the new option:
Result:
The dry run emitted expected local warnings for unavailable Slurm commands, but hook discovery succeeded and the complete NCCL scenario was generated.
Additional Notes