Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Jul 11, 2025

Pytest requires a clean envrionment with imports otherwise it throws errors. This way codeflash's imports should not interfere with the actual traced program. All of the heavy codeflash work can happen in the tracer.py and all the work in the user program can happen in tracer_new_process.py

PR Type

Enhancement


Description

  • Delegate tracing to isolated subprocess

  • Remove legacy inline tracer implementation

  • Introduce new tracing_new_process.py script

  • Enhance file‐filtering in tracing_utils.py


Changes diagram

flowchart LR
  A["`codeflash/tracer.py` main"] -- "spawn subprocess" --> B["`tracing_new_process.py`"]
  B -- "write result pickle" --> C["result pickle file"]
  C -- "load replay test path" --> A
Loading

Changes walkthrough 📝

Relevant files
Enhancement
tracer.py
Delegate tracer to subprocess                                                       

codeflash/tracer.py

  • Removed ~800 lines of inline tracer logic
  • Added subprocess.run using SAFE_SYS_EXECUTABLE
  • Built args_dict for external process invocation
  • Simplified main() to delegate tracing
  • +64/-803
    tracing_new_process.py
    Add isolated tracer process script                                             

    codeflash/tracing/tracing_new_process.py

  • New script encapsulating full tracer logic
  • Defines FakeCode, FakeFrame, and Tracer classes
  • Profiles, writes SQLite trace, dumps pickle result
  • Handles replay test generation and cleanup
  • +863/-0 
    tracing_utils.py
    Enhance tracing utilities                                                               

    codeflash/tracing/tracing_utils.py

  • Switched to standard @dataclass for FunctionModules
  • Added site‐package path checks and Git submodule support
  • Introduced path_belongs_to_site_packages, is_git_repo
  • Cached ignored_submodule_paths and improved filtering
  • +55/-2   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    github-actions bot commented Jul 11, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 3e8bbf2)

    Here are some key observations to aid the review process:

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

    Missing Import

    The script references importlib.machinery.ModuleSpec but does not import that submodule, causing a runtime AttributeError. Add an explicit import for ModuleSpec.

        import runpy
    
        code = "run_module(modname, run_name='__main__')"
        globs = {"run_module": runpy.run_module, "modname": args_dict["progname"]}
    else:
        sys.path.insert(0, str(Path(args_dict["progname"]).resolve().parent))
        with io.open_code(args_dict["progname"]) as fp:
            code = compile(fp.read(), args_dict["progname"], "exec")
        spec = importlib.machinery.ModuleSpec(name="__main__", loader=None, origin=args_dict["progname"])
        globs = {
    Return Value Mismatch

    discover_unit_tests now returns three values but some strategies (e.g., discover_tests_unittest) may still return only two, leading to unpacking errors. Ensure all discovery strategies return the expected tuple of three elements.

        cfg: TestConfig,
        discover_only_these_tests: list[Path] | None = None,
        file_to_funcs_to_optimize: dict[Path, list[FunctionToOptimize]] | None = None,
    ) -> tuple[dict[str, set[FunctionCalledInTest]], int, int]:
        framework_strategies: dict[str, Callable] = {"pytest": discover_tests_pytest, "unittest": discover_tests_unittest}
        strategy = framework_strategies.get(cfg.test_framework, None)
        if not strategy:
            error_message = f"Unsupported test framework: {cfg.test_framework}"
            raise ValueError(error_message)
    Subprocess Error Handling

    The subprocess.run call is invoked with check=False and no output capture, so failures or missing pickle files may go unnoticed. Consider checking the exit status, capturing stdout/stderr, and handling absent result files gracefully.

    config, found_config_path = parse_config_file(parsed_args.codeflash_config)
    project_root = project_root_from_module_root(Path(config["module_root"]), found_config_path)
    if len(unknown_args) > 0:
        try:
            result_pickle_file_path = get_run_tmp_file("tracer_results_file.pkl")
            args_dict = {
                "result_pickle_file_path": str(result_pickle_file_path),
                "output": str(parsed_args.outfile),
                "functions": parsed_args.only_functions,
                "disable": False,
                "project_root": str(project_root),
                "max_function_count": parsed_args.max_function_count,
                "timeout": parsed_args.tracer_timeout,
                "command": " ".join(sys.argv),
                "progname": unknown_args[0],
                "config": config,
                "module": parsed_args.module,
            }
    
            subprocess.run(
                [
                    SAFE_SYS_EXECUTABLE,
                    Path(__file__).parent / "tracing" / "tracing_new_process.py",
                    *sys.argv,
                    json.dumps(args_dict),
                ],
                cwd=Path.cwd(),
                check=False,
            )

    @github-actions
    Copy link

    github-actions bot commented Jul 11, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 3e8bbf2
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add missing type imports

    Add the imports for Path and Any so that the TraceConfig annotations are recognized.
    Without these, Path and Any will be undefined in the module.

    tests/test_tracer.py [2-3]

     import dataclasses
     import pickle
    +from pathlib import Path
    +from typing import Any
    Suggestion importance[1-10]: 9

    __

    Why: The annotations in TraceConfig rely on Path and Any, so missing imports would cause NameError at runtime.

    High
    Handle strategy return size

    Make the unpack of strategy results robust to handle both two- and three-value
    returns. This prevents unpack errors when the unittest strategy still returns two
    items.

    codeflash/discovery/discover_unit_tests.py [355-357]

    -function_to_tests, num_discovered_tests, num_discovered_replay_tests = strategy(
    -    cfg, discover_only_these_tests, functions_to_optimize
    -)
    +res = strategy(cfg, discover_only_these_tests, functions_to_optimize)
    +if len(res) == 3:
    +    function_to_tests, num_discovered_tests, num_discovered_replay_tests = res
    +else:
    +    function_to_tests, num_discovered_tests = res
    +    num_discovered_replay_tests = 0
    Suggestion importance[1-10]: 7

    __

    Why: The unpacking of strategy’s return into three variables may fail if any strategy still returns two values, so adding a length check prevents runtime errors and supports backward compatibility.

    Medium
    General
    Improve subprocess call handling

    Convert Path objects to strings in the subprocess arguments, enable error checking,
    and wrap the call in a try-except block to catch failures and exit with an
    informative message.

    codeflash/tracer.py [105-113]

    -subprocess.run(
    -    [
    -        SAFE_SYS_EXECUTABLE,
    -        Path(__file__).parent / "tracing" / "tracing_new_process.py",
    -        *sys.argv,
    -        json.dumps(args_dict),
    -    ],
    -    cwd=Path.cwd(),
    -    check=False,
    -)
    +try:
    +    subprocess.run(
    +        [
    +            SAFE_SYS_EXECUTABLE,
    +            str(Path(__file__).parent / "tracing" / "tracing_new_process.py"),
    +            *sys.argv,
    +            json.dumps(args_dict),
    +        ],
    +        cwd=str(Path.cwd()),
    +        check=True,
    +    )
    +except subprocess.CalledProcessError as e:
    +    console.print(f"Tracer process failed: {e}")
    +    sys.exit(1)
    Suggestion importance[1-10]: 7

    __

    Why: Wrapping subprocess.run in a try-except and converting paths to strings improves error reporting and ensures proper invocation of the tracer subprocess.

    Medium
    Use context manager for pickle

    Use a context manager when opening the pickle file so the file handle is closed
    automatically after reading.

    tests/test_tracer.py [419]

    -pickled = pickle.load(trace_config.result_pickle_file_path.open("rb"))
    +with trace_config.result_pickle_file_path.open("rb") as f:
    +    pickled = pickle.load(f)
    Suggestion importance[1-10]: 6

    __

    Why: Wrapping the file open in a with statement ensures the file handle is automatically closed, improving resource management.

    Low
    Correct fixture return annotation

    Update the fixture’s return type annotation to Generator[TraceConfig, None, None] so
    that tools and readers see that it yields a TraceConfig object rather than a Path.

    tests/test_tracer.py [62]

    -def trace_config(self) -> Generator[Path, None, None]:
    +def trace_config(self) -> Generator[TraceConfig, None, None]:
    Suggestion importance[1-10]: 5

    __

    Why: Updating the fixture return type to Generator[TraceConfig, None, None] clarifies the yielded object type but does not affect runtime behavior.

    Low

    Previous suggestions

    Suggestions up to commit 6394415
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate subprocess success before using data

    Check the subprocess return code before reading and using the pickle file, and bail
    out early on failure to avoid referencing an undefined data variable. You can set
    check=True in subprocess.run or explicitly inspect result.returncode and exit on
    non-zero status.

    codeflash/tracer.py [126-148]

     result = subprocess.run(
         [
             SAFE_SYS_EXECUTABLE,
    -        Path(__file__).parent / "tracing" / "tracing_new_process.py",
    +        str(Path(__file__).parent / "tracing" / "tracing_new_process.py"),
             *sys.argv,
             json.dumps(args_dict),
         ],
         cwd=os.getcwd(),
    -    check=False,
    +    check=True,
     )
    -try:
    -    with result_pickle_file_path.open(mode="rb") as f:
    -        data = pickle.load(f)
    -except Exception:
    -    print("Failed to trace")
    -    exitcode = -1
    -finally:
    -    result_pickle_file_path.unlink(missing_ok=True)
    +with result_pickle_file_path.open(mode="rb") as f:
    +    data = pickle.load(f)
    +result_pickle_file_path.unlink(missing_ok=True)
     
     replay_test_path = data["replay_test_file_path"]
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring subprocess.run uses check=True prevents referencing undefined data when the subprocess fails, avoiding potential runtime errors.

    Medium
    General
    Remove debug prints

    Remove these debug print statements in init and enter to avoid cluttering
    the traced program's output and leaking internal state. Use structured logging or
    the existing console interface if you need diagnostics.

    codeflash/tracing/tracing_new_process.py [96-205]

    -print(locals())
    -# ...
    -print(dir(self))
    -for att in dir(self):
    -    if att[:2] != "__":
    -        print(att, getattr(self, att))
    +# debug prints removed for cleaner output
    Suggestion importance[1-10]: 4

    __

    Why: The print(locals()) and print(dir(self)) debug statements clutter output and leak internal state, and should be removed or replaced with structured logging.

    Low
    Use cached submodule paths directly

    Initialize and cache submodule_paths outside the conditional instead of resetting it
    to None on each call. This ensures the cached paths from ignored_submodule_paths are
    always used and avoids redundant checks.

    codeflash/tracing/tracing_utils.py [49-68]

     def filter_files_optimized(file_path: Path, tests_root: Path, ignore_paths: list[Path], module_root: Path) -> bool:
         """Optimized version of the filter_functions function above."""
    -    submodule_paths = None
    +    submodule_paths = ignored_submodule_paths(module_root)
         if file_path.is_relative_to(tests_root):
             return False
         # ...
    -    if submodule_paths is None:
    -        submodule_paths = ignored_submodule_paths(module_root)
         return not (
             file_path in submodule_paths
             or any(file_path.is_relative_to(submodule_path) for submodule_path in submodule_paths)
         )
    Suggestion importance[1-10]: 3

    __

    Why: Preloading submodule_paths is a micro-optimization but offers negligible benefit since ignored_submodule_paths is already cached.

    Low

    @misrasaurabh1 misrasaurabh1 marked this pull request as ready for review July 14, 2025 01:29
    @github-actions
    Copy link

    Persistent review updated to latest commit 3e8bbf2

    @misrasaurabh1
    Copy link
    Contributor Author

    force merging this in since i want to deploy this for pydantic-ai

    @misrasaurabh1 misrasaurabh1 merged commit c288419 into main Jul 14, 2025
    17 of 18 checks passed
    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.

    1 participant