Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Jun 11, 2025

PR Type

Bug fix, Enhancement


Description

  • Relax --benchmarks-root subdirectory requirement

  • Create replay tests under tests_root directory


Changes walkthrough 📝

Relevant files
Error handling
cli.py
Remove benchmarks_root subdir assertion                                   

codeflash/cli_cmds/cli.py

  • Removed assertion enforcing benchmarks_root under tests_root
  • No longer checks subdirectory relationship between roots
  • +0/-3     
    Bug fix
    optimizer.py
    Save replay tests under tests_root                                             

    codeflash/optimization/optimizer.py

  • Changed tempfile.mkdtemp directory to use tests_root
  • Ensures replay tests saved in tests directory, not benchmarks
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @aseembits93 aseembits93 marked this pull request as draft June 11, 2025 23:43
    @github-actions
    Copy link

    github-actions bot commented Jun 11, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit b6a68ed)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Path validation relaxed

    The removal of the check enforcing benchmarks_root as a subdirectory of tests_root could lead to unexpected file path handling. Ensure downstream operations correctly support benchmark roots outside the tests directory.

    assert Path(args.module_root).is_dir(), f"--module-root {args.module_root} must be a valid directory"
    assert args.tests_root is not None, "--tests-root must be specified"
    assert Path(args.tests_root).is_dir(), f"--tests-root {args.tests_root} must be a valid directory"
    if args.benchmark:
        assert args.benchmarks_root is not None, "--benchmarks-root must be specified when running with --benchmark"
        assert Path(args.benchmarks_root).is_dir(), (
            f"--benchmarks-root {args.benchmarks_root} must be a valid directory"
        )
    Temporary directory cleanup

    tempfile.mkdtemp creates a new replay tests directory under tests_root but no cleanup mechanism is shown. Confirm whether these directories should be removed after use to avoid resource leaks.

    self.replay_tests_dir = Path(
        tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=self.args.tests_root)
    )

    @github-actions
    Copy link

    github-actions bot commented Jun 11, 2025

    PR Code Suggestions ✨

    Latest suggestions up to b6a68ed
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use TemporaryDirectory for cleanup

    Consider using TemporaryDirectory to manage the lifecycle of the replay tests folder
    automatically, ensuring it is cleaned up when no longer needed.

    codeflash/optimization/optimizer.py [130-132]

    -self.replay_tests_dir = Path(
    -    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=self.args.tests_root)
    +self._tempdir = tempfile.TemporaryDirectory(
    +    prefix="codeflash_replay_tests_", dir=self.args.tests_root
     )
    +self.replay_tests_dir = Path(self._tempdir.name)
    Suggestion importance[1-10]: 7

    __

    Why: Switching to TemporaryDirectory ensures automatic cleanup of the replay directory, improving resource management without altering core logic.

    Medium

    Previous suggestions

    Suggestions up to commit b6a68ed
    CategorySuggestion                                                                                                                                    Impact
    Security
    Revalidate benchmarks_root location

    Reinstate the check ensuring that benchmarks_root is within tests_root to prevent
    misconfiguration and unexpected file writes. This safeguards that benchmarks run
    only inside the designated tests directory.

    codeflash/cli_cmds/cli.py [135-140]

     if args.benchmark:
         assert args.benchmarks_root is not None, "--benchmarks-root must be specified when running with --benchmark"
         assert Path(args.benchmarks_root).is_dir(), (
             f"--benchmarks-root {args.benchmarks_root} must be a valid directory"
         )
    +    assert Path(args.benchmarks_root).resolve().is_relative_to(Path(args.tests_root).resolve()), (
    +        f"--benchmarks-root {args.benchmarks_root} must be a subdirectory of --tests-root {args.tests_root}"
    +    )
         if env_utils.get_pr_number() is not None:
    Suggestion importance[1-10]: 7

    __

    Why: The reinstated is_relative_to assertion correctly enforces that benchmarks_root is a subdirectory of tests_root, preventing misconfiguration and unintended file writes.

    Medium
    General
    Resolve tests_root before mkdtemp

    Resolve tests_root to an absolute path before passing it to mkdtemp to avoid
    creating directories in unexpected locations when relative paths are used.

    codeflash/optimization/optimizer.py [130-132]

    +base_tests_root = Path(self.args.tests_root).resolve()
     self.replay_tests_dir = Path(
    -    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=self.args.tests_root)
    +    tempfile.mkdtemp(prefix="codeflash_replay_tests_", dir=str(base_tests_root))
     )
    Suggestion importance[1-10]: 6

    __

    Why: Resolving tests_root ensures mkdtemp creates the replay directory at the intended absolute path, avoiding unexpected directory locations.

    Low

    @aseembits93
    Copy link
    Contributor Author

    Screenshot 2025-06-11 at 4 48 40 PM

    @aseembits93 aseembits93 marked this pull request as ready for review June 11, 2025 23:55
    @github-actions
    Copy link

    Persistent review updated to latest commit b6a68ed

    @aseembits93 aseembits93 requested a review from KRRT7 June 12, 2025 00:38
    @aseembits93 aseembits93 merged commit e10630d into main Jun 12, 2025
    16 checks passed
    @aseembits93 aseembits93 deleted the replay-test-save-dir branch June 12, 2025 00:46
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants