Skip to content

Allow external callers to configure the hook directory #951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shreyaskommuri wants to merge 1 commit into
base: main
Choose a base branch
from
+62 −16
Open

Allow external callers to configure the hook directory #951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions src/cloudai/cli/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# SPDX-FileCopyrightText: NVIDIA CORPORATION & AFFILIATES
# Copyright (c) 2024-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# Copyright (c) 2024-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -104,6 +104,13 @@ def common_options(f):
type=click.Path(exists=True, resolve_path=True, path_type=Path),
help="Scenario config path.",
)(f)
f = click.option(
"--hook-dir",
required=False,
default=None,
type=click.Path(exists=True, resolve_path=True, path_type=Path, file_okay=False, dir_okay=True),
help="Directory with hook scenario and test configs.",
)(f)
return f


Expand Down Expand Up @@ -136,18 +143,28 @@ def main(log_file, log_level):

@main.command()
@common_options
def install(system_cfg: Path, tests_dir: Path, scenario_cfg: Path):
def install(system_cfg: Path, tests_dir: Path, scenario_cfg: Path, hook_dir: Path | None):
"""Install the necessary components for workloads."""
args = argparse.Namespace(system_config=system_cfg, tests_dir=tests_dir, test_scenario=scenario_cfg, mode="install")
args = argparse.Namespace(
system_config=system_cfg,
tests_dir=tests_dir,
test_scenario=scenario_cfg,
hook_dir=hook_dir,
mode="install",
)
exit(handle_install_and_uninstall(args))


@main.command()
@common_options
def uninstall(system_cfg: Path, tests_dir: Path, scenario_cfg: Path):
def uninstall(system_cfg: Path, tests_dir: Path, scenario_cfg: Path, hook_dir: Path | None):
"""Uninstall the components used by workloads."""
args = argparse.Namespace(
system_config=system_cfg, tests_dir=tests_dir, test_scenario=scenario_cfg, mode="uninstall"
system_config=system_cfg,
tests_dir=tests_dir,
test_scenario=scenario_cfg,
hook_dir=hook_dir,
mode="uninstall",
)
exit(handle_install_and_uninstall(args))

Expand All @@ -161,6 +178,7 @@ def dry_run(
system_cfg: Path,
tests_dir: Path,
scenario_cfg: Path,
hook_dir: Path | None,
output_dir: Path,
enable_cache_without_check: bool,
single_sbatch: bool,
Expand All @@ -170,6 +188,7 @@ def dry_run(
system_config=system_cfg,
tests_dir=tests_dir,
test_scenario=scenario_cfg,
hook_dir=hook_dir,
output_dir=output_dir,
mode="dry-run",
enable_cache_without_check=enable_cache_without_check,
Expand All @@ -187,6 +206,7 @@ def run(
system_cfg: Path,
tests_dir: Path,
scenario_cfg: Path,
hook_dir: Path | None,
output_dir: Path,
enable_cache_without_check: bool,
single_sbatch: bool,
Expand All @@ -200,6 +220,7 @@ def run(
system_config=system_cfg,
tests_dir=tests_dir,
test_scenario=scenario_cfg,
hook_dir=hook_dir,
output_dir=output_dir,
mode="run",
enable_cache_without_check=enable_cache_without_check,
Expand All @@ -216,15 +237,19 @@ def run(
type=click.Path(exists=True, resolve_path=True, path_type=Path, file_okay=False),
help="Path to a scenario results directory.",
)
def generate_report(system_cfg: Path, tests_dir: Path, scenario_cfg: Path, result_dir: Path):
def generate_report(system_cfg: Path, tests_dir: Path, scenario_cfg: Path, hook_dir: Path | None, result_dir: Path):
"""
Generate a report from the results of a scenario.

While this process is automatically executed as part of "run" command, one can also invoke it manually using this
command.
"""
args = argparse.Namespace(
system_config=system_cfg, tests_dir=tests_dir, test_scenario=scenario_cfg, result_dir=result_dir
system_config=system_cfg,
tests_dir=tests_dir,
test_scenario=scenario_cfg,
hook_dir=hook_dir,
result_dir=result_dir,
)
exit(handle_generate_report(args))

Expand Down
6 changes: 3 additions & 3 deletions src/cloudai/cli/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def handle_install_and_uninstall(args: argparse.Namespace) -> int:
Args:
args (argparse.Namespace): The parsed command-line arguments.
"""
parser = Parser(args.system_config)
parser = Parser(args.system_config, args.hook_dir or HOOK_ROOT)
system, tests, scenario = parser.parse(args.tests_dir, args.test_scenario)

system.update()
Expand Down Expand Up @@ -249,7 +249,7 @@ def register_signal_handlers(signal_handler: Callable) -> None:
def _setup_system_and_scenario(
args: argparse.Namespace,
) -> tuple[System, TestScenario, list[TestDefinition]] | None:
parser = Parser(args.system_config)
parser = Parser(args.system_config, args.hook_dir or HOOK_ROOT)
try:
system, tests, test_scenario = parser.parse(args.tests_dir, args.test_scenario)
except MissingTestError as e:
Expand Down Expand Up @@ -354,7 +354,7 @@ def handle_generate_report(args: argparse.Namespace) -> int:
Args:
args (argparse.Namespace): The parsed command-line arguments.
"""
parser = Parser(args.system_config)
parser = Parser(args.system_config, args.hook_dir or HOOK_ROOT)
system, _, test_scenario = parser.parse(args.tests_dir, args.test_scenario)
assert test_scenario is not None

Expand Down
17 changes: 11 additions & 6 deletions src/cloudai/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,18 @@
class Parser:
"""Main parser for parsing all types of configurations."""

def __init__(self, system_config_path: Path) -> None:
def __init__(self, system_config_path: Path, hook_root: Path = HOOK_ROOT) -> None:
"""
Initialize a Parser instance.

Args:
system_config_path (str): The file path for system configurations.
hook_root (Path): Directory containing hook scenarios and tests.
"""
logging.debug(f"Initializing parser with: {system_config_path=}")
self.system_config_path = system_config_path
self.hook_root = hook_root
self.hook_test_root = hook_root / "test"
self._system: Optional[System] = None

@property
Expand Down Expand Up @@ -90,12 +93,14 @@ def parse(
except TestConfigParsingError:
exit(1) # exit right away to keep error message readable for users

if not HOOK_ROOT.exists():
logging.debug(f"HOOK_ROOT path '{HOOK_ROOT}' does not exist.")
if not self.hook_root.exists():
logging.debug(f"Hook root path '{self.hook_root}' does not exist.")

try:
hook_tests = (
self.parse_tests(list(HOOK_TEST_ROOT.glob("*.toml")), self.system) if HOOK_TEST_ROOT.exists() else []
self.parse_tests(list(self.hook_test_root.glob("*.toml")), self.system)
if self.hook_test_root.exists()
else []
)
except TestConfigParsingError:
exit(1) # exit right away to keep error message readable for users
Expand All @@ -106,10 +111,10 @@ def parse(

test_mapping = {t.name: t for t in tests}
hook_test_scenario_mapping = {}
if HOOK_ROOT.exists() and list(HOOK_ROOT.glob("*.toml")):
if self.hook_root.exists() and list(self.hook_root.glob("*.toml")):
try:
hook_test_scenario_mapping = self.parse_hooks(
list(HOOK_ROOT.glob("*.toml")), self.system, {t.name: t for t in hook_tests}
list(self.hook_root.glob("*.toml")), self.system, {t.name: t for t in hook_tests}
)
except TestScenarioParsingError:
exit(1) # exit right away to keep error message readable for users
Expand Down
9 changes: 9 additions & 0 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ def test_tests_dir_is_optional(tmp_path: Path):
assert "Missing option '--tests-dir'" not in result.output


def test_hook_dir_is_available_on_run_commands():
runner = CliRunner()

result = runner.invoke(main, ["dry-run", "--help"])

assert result.exit_code == 0
assert "--hook-dir DIRECTORY" in result.output
Comment on lines +49 to +55

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover all commands that now accept --hook-dir.

This only locks the help text for dry-run, but the PR contract is broader (install, uninstall, run, dry-run, generate-report). If one command later drops @common_options, this regression test still passes. Please parametrize this check over the full command set.

🤖 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 `@tests/test_cli.py` around lines 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.



@pytest.mark.parametrize(
"subcommand", ["dry-run", "generate-report", "install", "list", "run", "uninstall", "verify-configs"]
)
Expand Down
7 changes: 7 additions & 0 deletions tests/test_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ def test_no_tests_dir(self, parser: Parser):
parser.parse(tests_dir, None)
assert "Test path" in str(exc_info.value)

def test_custom_hook_root_is_used(self, parser: Parser, tmp_path: Path):
hook_root = tmp_path / "hooks"
parser = Parser(parser.system_config_path, hook_root)

assert parser.hook_root == hook_root
assert parser.hook_test_root == hook_root / "test"
Comment on lines +42 to +47

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Exercise parse() with the custom hook root, not just constructor state.

These assertions still pass if Parser.parse() regresses to consulting HOOK_ROOT directly. The behavior change in this PR is path resolution during parsing, so the regression test should drive parse() and verify the injected hook directory is the one actually used.

🤖 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 `@tests/test_parser.py` around lines 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.


@patch("cloudai.test_parser.TestParser.parse_all")
def test_no_scenario(self, test_parser: Mock, parser: Parser):
tests_dir = parser.system_config_path.parent.parent / "test"
Expand Down
Loading