Skip to content

Conversation

@tgasser-nv
Copy link
Collaborator

@tgasser-nv tgasser-nv commented Dec 11, 2025

Summary

In unit-tests, the AIPerfConfig output_base_dir was set to test_output in the pytest feature create_config_data(). In two tests, the config was used and created directories below the PWD. This fix changes each test to default output_base_dir to a temporary directory created by pytest using the tmp_path fixture.

Description

Before the change

$ poetry run pytest -q
$ ls -1 test_output/test_batch/20251211_190139  # Timestamp will match run time
concurrency1
concurrency2
process_result.json
run_metadata.json

After the change

$ poetry run pytest -q
$ ls -1 test_output
ls: test_output: No such file or directory

Related Issue(s)

#1501

Checklist

  • I've read the CONTRIBUTING guidelines.
  • I've updated the documentation if applicable.
  • I've added tests if applicable.
  • @mentions of the person or team responsible for reviewing proposed changes.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 11, 2025

Greptile Overview

Greptile Summary

Fixed temporary file cleanup issue in AIPerf unit tests by switching from hardcoded "test_output" directory to pytest's tmp_path fixture, ensuring test artifacts are automatically cleaned up after test execution.

  • Updated create_config_data fixture to accept and use tmp_path parameter
  • Changed default output_base_dir from hardcoded string to str(tmp_path)
  • Updated test assertion in test_init_with_valid_config to use str(config_file.parent) instead of hardcoded path
  • Minor formatting fix in streamers.py (moved type ignore comment)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are minimal and focused on test hygiene, using pytest's built-in tmp_path fixture which is the standard approach for temporary file handling in tests. All test assertions have been properly updated to reflect the new dynamic paths. No production code is affected except for a minor formatting change.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tests/benchmark/test_run_aiperf.py 5/5 Updated test fixtures to use pytest's tmp_path for temporary directories instead of hardcoded paths
nemoguardrails/llm/providers/huggingface/streamers.py 5/5 Moved type ignore comment to import line for better formatting compliance

Sequence Diagram

sequenceDiagram
    participant Test as Test Suite
    participant Fixture as create_config_data
    participant TmpPath as pytest tmp_path
    participant Config as AIPerfConfig
    participant FS as File System

    Test->>Fixture: Request config data
    Fixture->>TmpPath: Get temporary directory path
    TmpPath-->>Fixture: Return tmp_path
    Fixture->>Fixture: Set output_base_dir=str(tmp_path)
    Fixture-->>Test: Return config with temp path
    Test->>Config: Initialize AIPerfRunner
    Config->>FS: Create output directories under tmp_path
    FS-->>Config: Directories created
    Test->>Test: Run test assertions
    Test->>TmpPath: Test complete
    TmpPath->>FS: Auto-cleanup temp directories
    Note over FS: Temporary files removed automatically
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codecov
Copy link

codecov bot commented Dec 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Pouyanpi Pouyanpi merged commit 5c0c461 into develop Dec 12, 2025
7 checks passed
@Pouyanpi Pouyanpi deleted the fix/aiperf-test-tempfiles branch December 12, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants