Skip to content

RFC: Benchmarking scenarios #99

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.

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sjmonson
Copy link
Collaborator

@sjmonson sjmonson commented Mar 19, 2025

This PR adds support for "scenarios" that allow specifying benchmark argument in a file / as a single Pydantic object. CLI argument defaults are loaded from the scenario object defaults to give benchmark-as-code users the same defaults as CLI. Argument values in the CLI follow the following precedence: Scenario (class defaults) < Scenario (CLI provided Scenario) < CLI Arguments.

This PR is not in a finalized state but comments on design are encouraged.

Closes:

@sjmonson sjmonson changed the title Benchmarking scenarios RFC: Benchmarking scenarios May 1, 2025
@markurtz
Copy link
Member

@sjmonson I like the direction of this. A few quick thoughts from my side:

  • I think using Pydantic's built-in support for dotenv files along the lines of what we're doing for the general settings might work well here, and we can store the different scenarios then as just .env files and also enable potentially a default .env file: https://docs.pydantic.dev/latest/concepts/pydantic_settings/#dotenv-env-support
  • I'd like to see if we could get a unified setup for validation of values, types, and auto conversion that click is handling currently standardized through this, so all entry point pathways go through the same validations and fail early
  • It also would be great if we could get the Python API entrypoint and the CLI then pulling from the same defaults and able to support the same pathways

Not sure if there's something out there already to automatically handle the last two points, but if so, that would be a great inclusion

@sjmonson
Copy link
Collaborator Author

My plan was to rely on the base class json and yaml loaders since they are cleaner for nested structures, such as synthetic dataset args. I can definitely try pydantic-settings since that does have the advantage of allowing us to unify all GuideLLM options under one file.

  • I'd like to see if we could get a unified setup for validation of values, types, and auto conversion that click is handling currently standardized through this, so all entry point pathways go through the same validations and fail early

By default pydantic will attempt type coercion during the validation so its probably as simple as disabling the type handling in click and raising a click.BadParameter on model validation errors.

  • It also would be great if we could get the Python API entrypoint and the CLI then pulling from the same defaults and able to support the same pathways

They already do in this patch as I have set every click default to pull from the scenario model, unless you mean something else?

@markurtz
Copy link
Member

@sjmonson, for the last one, yes, something else. Specifically the entrypoint for benchmarking Python API here: https://github.com/neuralmagic/guidellm/blob/main/src/guidellm/benchmark/entrypoints.py#L22. Towards @anmarques's previous issues with needing to set all argument values when not using the CLI. Also would help towards suporting both CLI and Python API for the scenarios

@sjmonson
Copy link
Collaborator Author

Ah I see. The workflow I imagined for @anmarques's use-case is to call the higher-level entrypoint benchmark_with_scenario since it handles the defaults on scenario instantiation so a call like the following would be the same as calling the CLI with only the required args:

result = await benchmark_with_scenario(
    GenerativeTextScenario(
        target="http://localhost:8000",
        data={
            "prompt_tokens": 128,
            "output_tokens": 128,
        },
        rate_type="sweep",
    ),
    output_path="output.json",
)

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