Support ShareGPT datasets#906
Conversation
Generated-by: Cursor AI Claude Opus 4.6 Signed-off-by: Jared O'Connell <joconnel@redhat.com>
dbutenhof
left a comment
dbutenhof
left a comment
There was a problem hiding this comment.
I've seen the unhandled errors when a dataset has splits we don't process -- it's ugly. I like the idea of assuming a default split, and "train" is certainly commin -- though I'm not sure all split datasets necessarily have a "train" split, and we should handle that case gracefully as long as we're trying to be graceful.
- I think we should handle this default unwrapping earlier, when we load the data -- basically, in the absence of
load_kwargs.split=<name>(when we receive a split dataset) we should resolve it right there. - If "train" is not in the split keys, we could use an arbitrary split that does exist or perhaps we should instead raise an error exception advising the user that they need to specify a split for that dataset.
| :return: A single Dataset or IterableDataset. | ||
| """ | ||
| if isinstance(dataset, DatasetDict | IterableDatasetDict): | ||
| if "train" in dataset: |
There was a problem hiding this comment.
This is great, although I suspect we should do this earlier when we first load the dataset. E.g., if you don't specify load_kwargs.split=<name> on --data, and the dataset has splits, apply this unwrapping right off rather than deferring it to here.
| return None | ||
|
|
||
| try: | ||
| parsed = json.loads(value) |
There was a problem hiding this comment.
Three separate methods using json.loads -- can we avoid the duplication by consolidating some of these paths??
SkiHatDuckie
left a comment
SkiHatDuckie
left a comment
There was a problem hiding this comment.
I haven't ran anything yet, but I did take a look at the code involved in detecting/unwrapping JSON dictionary columns. Regardless of what we choose to do with the column mapper, it'll help to make the unwrapper as general-purpose as possible, so that it may hopefully be useful regardless of the situation it's called in.
| if len(dataset_columns) != 1: | ||
| return None | ||
|
|
||
| candidate = dataset_columns[0] |
There was a problem hiding this comment.
While this will work for ShareGPT, there's no guarantee that the column(s!) containing JSON-dicts in other dataset formats will be in the first column.
| if not base_match: | ||
| continue | ||
| # Fallback: if no matches found, try JSON unwrapping | ||
| if not matched: |
There was a problem hiding this comment.
This won't work as intended if some, but not all of the target columns are found before checking wrappers.
| When a dataset has no direct column matches but contains a single | ||
| JSON-string column, the inner keys of that JSON are used as virtual | ||
| column names and matching is retried. | ||
|
|
There was a problem hiding this comment.
If we wish to standardize the JSON unwrapper/detector for use across GuideLLM, we'll have to make as few assumptions about the location(s) of target columns as possible.
A future dataset might have multiple columns with JSON-wrapped dicts, for example.
3 participants
Summary
Adds support for ShareGPT datasets. This is a format commonly used for tool call datasets.
These changes were pulled from my prior PR.
This change highlights some limitations of the current design, and as such I've put it in draft in case we decide to refactor the code. The current column mapper design results in two column mapping steps, basically.
Details
Test Plan
Use of AI
git log
commit 613dc9a
Author: Jared O'Connell joconnel@redhat.com
Date: Mon Jul 6 18:05:25 2026 -0400
Generated-by: Cursor AI Claude Opus 4.6
Signed-off-by: Jared O'Connell joconnel@redhat.com