adding the NIM LLM TCO calculator tool - #389
Conversation
ajcasagrande
left a comment
ajcasagrande
left a comment
There was a problem hiding this comment.
Lots of small comments and/or suggestions. Some questions regarding file placement and Google access. Other than that, looks good and nothing major.
| "```\n", | ||
| "export NGC_API_KEY=<YOUR_NGC_API_KEY> \n", | ||
| "\n", | ||
| "# Choose a container name for bookkeeping\n", |
There was a problem hiding this comment.
What do you mean by bookkeeping here?
There was a problem hiding this comment.
Terminology from the NIM documentation: https://docs.nvidia.com/nim/large-language-models/latest/getting-started.html
My interpretation is that it's the same as "logging".
There was a problem hiding this comment.
rewrote this portion and removed the comment. I think the name must be compliant with NGC and not just for bookkeeping
| ], | ||
| "source": [ | ||
| "%%writefile benchmark.sh\n", | ||
| "declare -A useCases\n", |
There was a problem hiding this comment.
Recommend adding missing shebang
#!/usr/bin/env bash| " local INPUT_SEQUENCE_STD=0\n", | ||
| " local OUTPUT_SEQUENCE_LENGTH=$outputLength\n", | ||
| " local CONCURRENCY=$concurrency\n", | ||
| " local MODEL=meta/llama-3.1-8b-instruct\n", |
There was a problem hiding this comment.
MODEL and --tokenizer are hard coded in the command here, however the user has the ability to choose the container name above, which could affect this. Recommend exposing the model/tokenizer as external env vars.
There was a problem hiding this comment.
moved to top of file
export MODEL=meta/llama-3.1-8b-instruct # NGC model name
export TOKENIZER_PATH=meta-llama/Meta-Llama-3-8B-Instruct # Either a HF model or path to a local folder containing the tokenizer
however user will still have to manually define this within the file, as model & tokenizer use slightly different model naming (ie. HF vs. NGC)
| "source": [ | ||
| "## Exporting data to excel format\n", | ||
| "\n", | ||
| "We next export the benchmarking data to a TCO-tool compatible format, which comprises both metadata fields as well as performance metric fields." |
There was a problem hiding this comment.
should this be TCO-tool or TCO-calculator compatible format?
| "root_dir = \"./artifacts\"\n", | ||
| "directory_prefix = \"meta_llama-3.1-8b-instruct-openai-chat-concurrency\" # Change this to fit the actual model deployed\n", | ||
| "\n", | ||
| "ISL_OSL_list = [\"200_5\", \"200_200\", \"1000_200\", \"200_1000\"]\n", |
There was a problem hiding this comment.
recommend full caps: ISL_OSL_LIST
| "directory_prefix = \"meta_llama-3.1-8b-instruct-openai-chat-concurrency\" # Change this to fit the actual model deployed\n", | ||
| "\n", | ||
| "ISL_OSL_list = [\"200_5\", \"200_200\", \"1000_200\", \"200_1000\"]\n", | ||
| "concurrencies = [1, 2, 5, 10, 50, 100, 250]\n", |
There was a problem hiding this comment.
consider capitalizing CONCURRENCIES to match ISL_OSL
| "df = pd.DataFrame(columns=gen_AI_perf_field)\n", | ||
| "\n", | ||
| "for con in concurrencies:\n", | ||
| " for ISL_OSL in ISL_OSL_list:\n", |
There was a problem hiding this comment.
recommend lowercase for local loop variables: for isl_osl in ISL_OSL_LIST:
| "concurrencies = [1, 2, 5, 10, 50, 100, 250]\n", | ||
| "df = pd.DataFrame(columns=gen_AI_perf_field)\n", | ||
| "\n", | ||
| "for con in concurrencies:\n", |
There was a problem hiding this comment.
Consider rename for concurrency in concurrencies:
Co-authored-by: Anthony Casagrande <acasagrande@nvidia.com>
Co-authored-by: Anthony Casagrande <acasagrande@nvidia.com>
debermudez
left a comment
debermudez
left a comment
There was a problem hiding this comment.
I think that @ajcasagrande did a great job reviewing this and I agree with his comments.
My concern is protecting this. I would like to see the contents of this migrated to a CI test to ensure we know asap if something breaks.
@ganeshku1 can we prioritize that?
|
Restructured and revised as suggested. Please review again. |
|
@ajcasagrande @debermudez do you mind reviewing this again? (The TCO spreadsheet calculator link is TBD but rest is good to go) |
|
@ganeshku1 what is the priority around ensuring the examples in here are covered in the CI? |
Since we're currently focused on AIPerf, this isn't a high priority right now. I don't anticipate any new features coming into GAP, so I’d prefer the team not invest significant effort here at the moment. Lets get this initial doc in and we will have to port over this to AIPerf, we will add CI as part of that porting work. |
4 participants
This tool allow exporting data from genAI-perf to the NIM LLM spreadsheet TCO calculator tool