Support payload input in GAP #219
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dyastremsky
force-pushed
the
feature-request-level-timing
branch
from
7ca4922 to
91f8ec1
Compare
lkomali
force-pushed
the
dbermudez-add-timing-retriever
branch
2 times, most recently
from
c829674 to
a143645
Compare
lkomali
force-pushed
the
dbermudez-add-timing-retriever
branch
from
a143645 to
10148f4
Compare
Replace with correct variable
debermudez
force-pushed
the
dbermudez-add-timing-retriever
branch
from
10148f4 to
ae3759c
Compare
lkomali
force-pushed
the
dbermudez-add-timing-retriever
branch
from
ae3759c to
82a3726
Compare
lkomali
force-pushed
the
dbermudez-add-timing-retriever
branch
2 times, most recently
from
82a3726 to
7f47060
Compare
lkomali
force-pushed
the
dbermudez-add-timing-retriever
branch
from
b5c5d71 to
3f6e35c
Compare
| with open(str(filename), "w") as f: | ||
| f.write(json.dumps(self._stats_and_args, indent=2)) | ||
|
|
||
| def _prepare_args_for_export(self) -> None: | ||
| self._args.pop("func", None) | ||
| self._args.pop("output_format", None) | ||
| self._args.pop("input_file", None) | ||
| self._args.pop("payload_input_file", None) |
There was a problem hiding this comment.
Aside to the team: I didn't know this was here. We should create a cleaner way to do this. Like having a separate function with a list of args that are excluded, then popping them all. We could get refactor the code to get rid of all of the args that aren't necessary for the JSON exporter to make it cleaner.
| @@ -56,6 +56,11 @@ def convert( | |||
| payload = { | |||
| "input": [{"type": "image_url", "url": img} for img in row.images] | |||
| } | |||
| request_body["data"].append({"payload": [payload]}) | |||
| self._add_payload_params(payload, row.optional_data) | |||
There was a problem hiding this comment.
I don't love this approach. Is there a way to wrap this and make it cleaner? This is going to make the converters less straightforward to write. You'll also need to update the customizable frontends tutorial to reflect any changes here.
There was a problem hiding this comment.
Do you mean for this converter? OR are you making a general statement about the way this was implemented? Just trying to understand the scope of the ask.
There was a problem hiding this comment.
All the converters. I think this is the "hot path" for adding a new converter (i.e. customizable frontend). We want it to be as simple, concise, and intuitive as possible.
Any changes to this process also need to be documented in customizable_frontends.md.
| if not filename.exists(): | ||
| raise FileNotFoundError(f"The file '{filename}' does not exist.") | ||
|
|
||
| def _get_content_from_input_file(self, filename: Path) -> Union[ |
There was a problem hiding this comment.
Do you want to organize these into the implemented ones vs the non-implemented ones? And perhaps alphabetize them within each of those sections for easy navigation?
There was a problem hiding this comment.
Sorry. This was also my mess. I agree that it needs to be cleaned up more.
genai-perf/genai_perf/inputs/retrievers/payload_input_retriever.py
Outdated
Show resolved
Hide resolved
genai-perf/genai_perf/inputs/retrievers/payload_input_retriever.py
Outdated
Show resolved
Hide resolved
genai-perf/genai_perf/inputs/retrievers/payload_input_retriever.py
Outdated
Show resolved
Hide resolved
lkomali
force-pushed
the
dbermudez-add-timing-retriever
branch
from
0b19aad to
482a3b7
Compare
lkomali
force-pushed
the
dbermudez-add-timing-retriever
branch
from
8a6b941 to
6096b2c
Compare
|
|
||
| if args.prompt_source == ic.PromptSource.PAYLOAD: | ||
| if args.concurrency or args.request_rate: | ||
| raise ValueError( |
There was a problem hiding this comment.
nit: Could you return this as a parser.error? That's more native to ArgParse and consistent with the other errors returned.
|
|
||
| Filename: TypeAlias = str | ||
| TypeOfData: TypeAlias = str | ||
| ListOfData: TypeAlias = List[str] | ||
| DataRowDict: TypeAlias = Dict[TypeOfData, ListOfData] | ||
| DataRowDict: TypeAlias = Dict[str, Union[List[str], Dict[str, Any], str]] |
There was a problem hiding this comment.
Maybe loop in our alias whiz, @nv-braf? My guess though is that if it's this complex, we're doing some overloading with typing that's really unclean and masking other issues. I also would have no idea what's going on here from a quick review of the code.
I can't speak to the complexity over time, which we have tried to reduce, but if you look at the before/after, one of these is relatively straightforward and the other is relatively messy. Having three possible datatypes values for a DataRowDict (or anything) doesn't sit right with me. That's too much overloading. We could fix the underlying issue in a future refactor, theoretically, but it looks like this PR is creating the above issue so we should fix it now.
| """ | ||
|
|
||
| files_data: Dict[str, FileData] = {} | ||
| input_file = cast(Path, self.config.payload_input_filename) |
There was a problem hiding this comment.
Okay, so it sounds like we need to clean this up somewhere. This is using latent knowledge of what the types should be, which is dangerous and hard to navigate as a code reader.
We can leave this given the other code already does this, but whenever we have a major refactor (e.g. for GA), we should look at removing these casts wherever possible. Ryan pointed this out to me in one of my reviews that I cleaned up: there was a type that was optional to make the code flow easier and he asked whether it ever should be optional; the answer was no, so even though it required more effort to think through the default, I swapped it to have the correct data type and only that type.
This PR creates a