Skip to content

Add proposal for unified results directory structure#79

Open
maryamtahhan wants to merge 2 commits intoredhat-et:mainfrom
maryamtahhan:results-dirs-naming-proposal
Open

Add proposal for unified results directory structure#79
maryamtahhan wants to merge 2 commits intoredhat-et:mainfrom
maryamtahhan:results-dirs-naming-proposal

Conversation

@maryamtahhan
Copy link
Copy Markdown
Collaborator

@maryamtahhan maryamtahhan commented Mar 30, 2026

This proposal addresses inconsistent results directories across test types and adds support for custom test run names as requested in issue #73.

Key changes:

  • Create docs/proposals/ directory for design proposals
  • Add results-directory-structure.md with detailed proposal
  • Update docs/docs.md to reference new proposal

The proposal includes:

  • Unified 6-level hierarchy for LLM tests (type/suite/model/workload/run/config)
  • Simplified 5-level hierarchy for embedding tests (type/scenario/model/run/config)
  • Custom test run names via parameter or environment variable
  • Migration strategy with 4 non-breaking phases
  • Streamlit visualization support for filtering by test run name

Summary by CodeRabbit

  • Documentation
    • Added a new "Design Proposals" section linking to active proposals and workflow documentation
    • Introduced standardized proposal template with defined workflow stages (Draft, Review, Approved, Implemented)
    • Added comprehensive design proposal for unified results directory structure for test outputs

This proposal addresses inconsistent results directories across test types
and adds support for custom test run names as requested in issue redhat-et#73.

Key changes:
- Create docs/proposals/ directory for design proposals
- Add results-directory-structure.md with detailed proposal
- Update docs/docs.md to reference new proposal

The proposal includes:
- Unified 6-level hierarchy for LLM tests (type/suite/model/workload/run/config)
- Simplified 5-level hierarchy for embedding tests (type/scenario/model/run/config)
- Custom test run names via parameter or environment variable
- Migration strategy with 4 non-breaking phases
- Streamlit visualization support for filtering by test run name

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

Introduces a new design proposals documentation system for the vLLM CPU Performance Evaluation framework. Establishes a proposals hub with standardized workflow stages and templates, followed by the first proposal defining a unified results directory structure for LLM and embedding test outputs with naming conventions, validation rules, and a phased migration strategy.

Changes

Cohort / File(s) Summary
Proposal Documentation System
docs/docs.md, docs/proposals/README.md, docs/proposals/results-directory-structure.md
Introduces design proposals documentation hub with workflow stages (Draft, Review, Approved, Implemented) and proposal template. First proposal specifies unified results directory hierarchy (results/llm/{test_suite}/{model_safe}/{workload}... and results/embedding/{scenario}/{model_safe}...) with required artifact filenames, environment variable support (TEST_RUN_NAME), character sanitization rules, test suite determination logic for LLM tests, and multi-phase migration plan. Includes Streamlit follow-up work requirements for results discovery, parsing, and UI filtering.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add proposal for unified results directory structure' directly and clearly summarizes the main change—adding a new design proposal document for standardizing results directory layouts, which aligns with the PR's core objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- Add language identifiers to code blocks to prevent comment lines
  from being treated as H1 headings (MD025)
- Restart numbered lists at 1 for each subsection (MD029)
- Convert comment-style annotations to plain text descriptions

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/docs.md`:
- Line 133: Update the stale date in the documentation table row that currently
reads "proposals/results-directory-structure.md | 🚧 In Review | 2024-03-30" to
the correct creation date "2026-03-30"; locate the string in docs/docs.md (the
table entry for proposals/results-directory-structure.md) and replace the year
portion of the date only so the row becomes
"proposals/results-directory-structure.md | 🚧 In Review | 2026-03-30".

In `@docs/proposals/results-directory-structure.md`:
- Around line 396-399: The checklist links in results-directory-structure.md
point one level too shallow; update each relative link in the checklist (the
entries referencing automation/test-execution/ansible/ansible.md, README.md,
results/results.md, and the migration guide line) to use the correct path from
docs/proposals (prefix with ../../) so they resolve to the repository root
(e.g., change ../automation/... to ../../automation/..., ../README.md to
../../README.md, and ../results/results.md to ../../results/results.md).
- Around line 106-112: Remove the stray opening code fence before the "### Path
Examples" heading that is breaking the markdown structure; locate the "### Path
Examples" and the subsequent "#### LLM Examples" section and delete the
unintended ``` so the headings render normally, and verify that the intended
fenced blocks (e.g., the ```text``` that follows "#### LLM Examples") are
properly balanced and only surround the intended example content.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 85930f4a-5bbd-44ff-be15-a6a18f0cf467

📥 Commits

Reviewing files that changed from the base of the PR and between 990c505 and b7ef26d.

📒 Files selected for processing (3)
  • docs/docs.md
  • docs/proposals/README.md
  • docs/proposals/results-directory-structure.md

| methodology/metrics.md | ✅ Complete | 2024-02-08 |
| methodology/reporting.md | ✅ Complete | 2024-02-08 |
| platform-setup/x86/intel/deterministic-benchmarking.md | ✅ Complete | (current) |
| proposals/results-directory-structure.md | 🚧 In Review | 2024-03-30 |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Update stale “Last Updated” year for the new proposal entry.

Line 133 uses 2024-03-30, but this PR was created on 2026-03-30. Keeping the year aligned avoids timeline confusion in the status table.

Suggested fix
-| proposals/results-directory-structure.md | 🚧 In Review | 2024-03-30 |
+| proposals/results-directory-structure.md | 🚧 In Review | 2026-03-30 |
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
| proposals/results-directory-structure.md | 🚧 In Review | 2024-03-30 |
| proposals/results-directory-structure.md | 🚧 In Review | 2026-03-30 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/docs.md` at line 133, Update the stale date in the documentation table
row that currently reads "proposals/results-directory-structure.md | 🚧 In
Review | 2024-03-30" to the correct creation date "2026-03-30"; locate the
string in docs/docs.md (the table entry for
proposals/results-directory-structure.md) and replace the year portion of the
date only so the row becomes "proposals/results-directory-structure.md | 🚧 In
Review | 2026-03-30".

Comment on lines +106 to +112
```

### Path Examples

#### LLM Examples

```text
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Remove stray code fence that breaks markdown structure.

Line 106 opens an unintended fenced block; this can swallow subsequent headings/content and render the document incorrectly.

Suggested fix
 #### Embedding Tests (5 levels)
 1. **Model Type** (`embedding/`) - Top-level categorization
 2. **Scenario** - Test scenario (baseline, latency, concurrent)
 3. **Model** - Specific model being tested
 4. **Run Name** - Unique identifier (custom or timestamp)
 5. **Config** - Configuration (if applicable - often just files at run_name level)
-```
 
 ### Path Examples
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/proposals/results-directory-structure.md` around lines 106 - 112, Remove
the stray opening code fence before the "### Path Examples" heading that is
breaking the markdown structure; locate the "### Path Examples" and the
subsequent "#### LLM Examples" section and delete the unintended ``` so the
headings render normally, and verify that the intended fenced blocks (e.g., the
```text``` that follows "#### LLM Examples") are properly balanced and only
surround the intended example content.

Comment on lines +396 to +399
- [ ] Update [automation/test-execution/ansible/ansible.md](../automation/test-execution/ansible/ansible.md)
- [ ] Update [README.md](../README.md)
- [ ] Update [results/results.md](../results/results.md) (if exists)
- [ ] Add migration guide
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix relative links in checklist (currently one level short).

From docs/proposals/results-directory-structure.md, links to repo-root docs should use ../../..., otherwise navigation breaks.

Suggested fix
-- [ ] Update [automation/test-execution/ansible/ansible.md](../automation/test-execution/ansible/ansible.md)
-- [ ] Update [README.md](../README.md)
-- [ ] Update [results/results.md](../results/results.md) (if exists)
+- [ ] Update [automation/test-execution/ansible/ansible.md](../../automation/test-execution/ansible/ansible.md)
+- [ ] Update [README.md](../../README.md)
+- [ ] Update [results/results.md](../../results/results.md) (if exists)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/proposals/results-directory-structure.md` around lines 396 - 399, The
checklist links in results-directory-structure.md point one level too shallow;
update each relative link in the checklist (the entries referencing
automation/test-execution/ansible/ansible.md, README.md, results/results.md, and
the migration guide line) to use the correct path from docs/proposals (prefix
with ../../) so they resolve to the repository root (e.g., change
../automation/... to ../../automation/..., ../README.md to ../../README.md, and
../results/results.md to ../../results/results.md).

@jharriga
Copy link
Copy Markdown
Collaborator

Providing users to specify names for test-runs as an environment variable is key.
Also need to ensure that those custom names are selectable when visualizing results in StreamLIT dashboards.
EXAMPLE USECASE: I want to run three back-to-back tests with the same GuideLLM Benchmark settings. Lets say these parameters:
guidellm benchmark --target http://localhost:8000 --processor /data/jharriga/GuideLLM-work/Models/Llama-3.2-1B-Instruct --data prompt_tokens=256,output_tokens=128 --profile concurrent --max-requests 100 --rate=8 --output-dir RESULTS_PI34_RATES_8

After the testing is complete these (and other) specific results should be selectable in the StreamLIT Dashboard. Ideally allowing a Comparative View to visualize multiple 'custom names' in a single Graph with selectable Metrics in a pull-down menu.

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.

2 participants