-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[TRTC-1922] [feat] Recipe db and its ingestion implementation for OPS #8990
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
base: main
Are you sure you want to change the base?
Changes from all commits
d7975ab
1d40bfe
347f1a8
772c2b7
f771dd1
9109dfc
82218be
680bd01
3274dd4
e0c2b45
2320616
e65b540
71208df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,8 @@ | |
| from tensorrt_llm.bench.utils.data import (create_dataset_from_stream, | ||
| initialize_tokenizer, | ||
| update_metadata_for_multimodal) | ||
| from tensorrt_llm.bench.utils.scenario import ( | ||
| prepare_llm_api_options_for_recipe, process_recipe_scenario) | ||
| from tensorrt_llm.llmapi import CapacitySchedulerPolicy | ||
| from tensorrt_llm.logger import logger | ||
| from tensorrt_llm.sampling_params import SamplingParams | ||
|
|
@@ -60,6 +62,16 @@ | |
| multiple=True, | ||
| help="Paths to custom module directories to import.", | ||
| ) | ||
| @optgroup.option( | ||
| "--recipe", | ||
| type=click.Path(exists=True, | ||
| readable=True, | ||
| path_type=Path, | ||
| resolve_path=True), | ||
| default=None, | ||
| help= | ||
| "Path to a recipe YAML file containing scenario and LLM API configuration. " | ||
| "CLI flags explicitly set will override recipe values.") | ||
| @optgroup.option( | ||
| "--extra_llm_api_options", | ||
| type=str, | ||
|
|
@@ -302,6 +314,20 @@ def throughput_command( | |
| options: GeneralExecSettings = get_general_cli_options(params, bench_env) | ||
| tokenizer = initialize_tokenizer(options.checkpoint_path) | ||
|
|
||
| # Process recipe scenario if present | ||
| cli_defaults = { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't we populate this with what we get from the CLI?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean the cli defaults or cli args expicitly specified by the user? basically there are 3 information sources for a given scenario arg (say, isl):
|
||
| 'concurrency': -1, | ||
| 'target_input_len': None, | ||
| 'target_output_len': None, | ||
| 'num_requests': 0, | ||
| 'tp': 1, | ||
| 'pp': 1, | ||
| 'ep': None, | ||
| 'streaming': False, | ||
| } | ||
| params, options, scenario = process_recipe_scenario(params, options, | ||
| bench_env, cli_defaults) | ||
|
|
||
| # Extract throughput-specific options not handled by GeneralExecSettings | ||
| max_batch_size = params.get("max_batch_size") | ||
| max_num_tokens = params.get("max_num_tokens") | ||
|
|
@@ -397,7 +423,17 @@ def throughput_command( | |
| exec_settings["settings_config"]["dynamic_max_batch_size"] = True | ||
|
|
||
| # LlmArgs | ||
| exec_settings["extra_llm_api_options"] = params.pop("extra_llm_api_options") | ||
| # Process recipe format if detected - extract llm_api_options only | ||
| # Priority: --extra_llm_api_options > --recipe | ||
| recipe_path = params.pop("recipe", None) | ||
| extra_llm_api_options_path = params.pop("extra_llm_api_options", None) | ||
| config_path = extra_llm_api_options_path if extra_llm_api_options_path else recipe_path | ||
| # Convert Path to string if needed | ||
| if config_path is not None: | ||
| config_path = str(config_path) | ||
| exec_settings["extra_llm_api_options"] = prepare_llm_api_options_for_recipe( | ||
| config_path, scenario) | ||
|
|
||
| exec_settings["iteration_log"] = options.iteration_log | ||
|
|
||
| # Construct the runtime configuration dataclass. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make a new option group for these, we can classify them as mutually exclusive. I don't think we should allow both to be specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally thought since recipe contains more information, we allow and prioritize the information in
extra_llm_api_optionswhen such a conflict arises - and print a warning clearly telling which source we're prioritizing.but i agree, its a lot easier to reduce the UX space here by making it mutually exclusive