Port VLM functionalities from main branch to the refactor branch #769
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def from_file(file_path: Path) -> List[Dict[str, str]]: | ||
| with open(file_path, "r") as file: | ||
| data = [load_json_str(line) for line in file] | ||
| def from_file(file_path: Path, output_format: OutputFormat) -> List[Dict[str, str]]: |
There was a problem hiding this comment.
I think the Dataset Retriever should have no concept of output format. Otherwise, we're moving back towards where we currently are with everything being interdependent. We might want to discuss the best way to proceed here. Perhaps we can add a separate from_image_file method that can get an image from a file and then use it in the LLM Inputs file to add that image to each input. Or follow another approach.
There was a problem hiding this comment.
That's a valid point, and I agree. But I would rather focus on porting the VLM functionalities for this PR as there's already too many changes happening in this PR. I can tag a ticket for decoupling output format from this method. How does that sound?
There was a problem hiding this comment.
I need to think on it. I know that this port was already difficult. At the same time, deferring some parts is going to make other ports harder and make it more likely this gets missed. Introducing clutter to this class seems like a mistake unless we're going to immediately submit another PR to fix this. So then that becomes one more ticket/PR we need to do.
src/c++/perf_analyzer/genai-perf/genai_perf/llm_inputs/dataset_retriever.py
Show resolved
Hide resolved
src/c++/perf_analyzer/genai-perf/genai_perf/llm_inputs/dataset_retriever.py
Show resolved
Hide resolved
src/c++/perf_analyzer/genai-perf/genai_perf/llm_inputs/dataset_retriever.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for doing this work! I left some comments, but we can file follow-on tickets to refactor these immediately since this PR is large. It'd be better to get the port done, then we can refactor out these similar practices again and find a design that works.
| def from_file(file_path: Path) -> List[Dict[str, str]]: | ||
| with open(file_path, "r") as file: | ||
| data = [load_json_str(line) for line in file] | ||
| def from_file(file_path: Path, output_format: OutputFormat) -> List[Dict[str, str]]: |
There was a problem hiding this comment.
I need to think on it. I know that this port was already difficult. At the same time, deferring some parts is going to make other ports harder and make it more likely this gets missed. Introducing clutter to this class seems like a mistake unless we're going to immediately submit another PR to fix this. So then that becomes one more ticket/PR we need to do.
src/c++/perf_analyzer/genai-perf/genai_perf/llm_inputs/llm_inputs.py
Outdated
Show resolved
Hide resolved
| @@ -101,14 +117,20 @@ def create_llm_inputs( | |||
| prompt_tokens_mean, | |||
| prompt_tokens_stddev, | |||
| num_of_output_prompts, | |||
| image_width_mean, | |||
There was a problem hiding this comment.
We can leave this for now... but same thing here. Maybe we need an image class. Adding all of these args replicates the issues we're trying to get rid of with this refactor. So this will need another refactor after this port.
src/c++/perf_analyzer/genai-perf/genai_perf/llm_inputs/dataset_retriever.py
Outdated
Show resolved
Hide resolved
This PR ports VLM support from #756 and #759 into this dev branch.