Fallback on dataset script if user wants to load default config #6498
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
src/datasets/load.py
Outdated
| @@ -1621,6 +1621,7 @@ def dataset_module_factory( | |||
| data_dir: Optional[str] = None, | |||
| data_files: Optional[Union[Dict, List, str, DataFilesDict]] = None, | |||
| trust_remote_code: Optional[bool] = None, | |||
| _ignore_default_config=False, | |||
There was a problem hiding this comment.
Why the leading underscore?
There was a problem hiding this comment.
I would suggest naming it differently, to make clear that config_name was passed or not: if config_name is passed, we don't even care whether we should ignore or not that DEFAULT_CONFIG_NAME is set.
Maybe no_config_name?
There was a problem hiding this comment.
The underscore is to signify this is an internal parameter (not supposed to be used by users)
There was a problem hiding this comment.
I think I like this name: if _require_default_config_name=True we require the builder class to have a default config name. If the user doesn't specify a config name we set it to True
There was a problem hiding this comment.
The underscore is to signify this is an internal parameter (not supposed to be used by users)
I mean, it is the first time I see a "private" function parameter... Wondering if that makes sense...
There was a problem hiding this comment.
It's not a good practice :/ but it's simple and does the job
I hesitated to not have the feature to use the parquet export if the user does specify a config name explicitly just because I don't like doing "private" arguments
src/datasets/load.py
Outdated
| with fs.open(f"datasets/{path}/{filename}", "r", revision=revision, encoding="utf-8") as f: | ||
| can_load_config_from_parquet_export = ( | ||
| "DEFAULT_CONFIG_NAME" not in f.read() or _ignore_default_config | ||
| ) |
There was a problem hiding this comment.
I think it would be better to refactor this code and avoid the network call and file read if the variable "_ignore_default_config" is True.
| try: | ||
| return HubDatasetModuleFactoryWithParquetExport( | ||
| path, download_config=download_config, revision=dataset_info.sha | ||
| ).get_module() | ||
| except _datasets_server.DatasetsServerError: | ||
| pass | ||
| # Otherwise we must use the dataset script if the user trusts it | ||
| trust_remote_code = resolve_trust_remote_code(trust_remote_code, path) |
There was a problem hiding this comment.
I guess this is no longer necessary?
There was a problem hiding this comment.
It's already called inside the HubDatasetModuleFactoryWithScript - no need to have it here
You mean if there is a DEFAULT_CONFIG_NAME defined in the script but the dataset only has one configuration ? We can't easily get the number of configs without running the python code so I don't think we can support detect this case |
|
Most datasets with a script don't define DEFAULT_CONFIG_NAME if there is only one configuration anyway. So there is no issue e.g. for |
There was a problem hiding this comment.
You mean if there is a DEFAULT_CONFIG_NAME defined in the script but the dataset only has one configuration ? We can't easily get the number of configs without running the python code so I don't think we can support detect this case
I was trying to mean the case where DEFAULT_CONFIG_NAME is None but there is only a single config in BUILDER_CONFIGS, with a name different from "default".
In this case we can detect if "DEFAULT_CONFIG_NAME" is not mentioned and use the Parquet export. If it is mentioned (and maybe it is set to None or to the single config) I consider that it may have multiple configs and fall back on using the script |
|
... but the user does not pass the config name. |
|
In this case we load the single configuration (this is how a DatasetBuilder works) |
|
see datasets/src/datasets/builder.py Line 532 in 2feaa58 |
Show benchmarksPyArrow==8.0.0 Show updated benchmarks!Benchmark: benchmark_array_xd.json
Benchmark: benchmark_getitem_100B.json
Benchmark: benchmark_indices_mapping.json
Benchmark: benchmark_iterating.json
Benchmark: benchmark_map_filter.json
Show updated benchmarks!Benchmark: benchmark_array_xd.json
Benchmark: benchmark_getitem_100B.json
Benchmark: benchmark_indices_mapping.json
Benchmark: benchmark_iterating.json
Benchmark: benchmark_map_filter.json
|
Right now this code is failing on
main:This is because it tries to load the dataset from the Parquet export but the dataset has multiple configurations and the Parquet export doesn't know which one is the default one.
I fixed this by simply falling back on using the dataset script (which tells the user to pass
trust_remote_code=True):Note that if the user happened to specify a config name I don't fall back on the script since we can use the Parquet export in this case (no need to know which config is the default)