Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename "configs" dataset card field to "config_names" #1491

Merged

Conversation

polinaeterna
Copy link
Contributor

@polinaeterna polinaeterna commented Jun 1, 2023

In datasets we want to use configs yaml field to define custom configurations' parameters for datasets without scripts: huggingface/datasets#5331 (comment)
So we decided to rename the old configs field to config_names cc @julien-c

@lhoestq already changed configs to config_names in all canonical datasets.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 1, 2023

The documentation is not available anymore as the PR was closed or merged.

@Wauplin
Copy link
Contributor

Wauplin commented Jun 1, 2023

Hi @polinaeterna, thanks for opening the PR. I'm fine with changing our naming as long as everyone agrees.
From a technical point of view, we would need to deprecate the configs parameter before removing it completely. Next version is 0.16 so let's say we definitely remove at version 0.19. Here is an example of deprecated argument:

@_deprecate_arguments(version="0.17", deprecated_args=["timeout"], custom_message="timeout is not used anymore.")
.

What you need to do:

  • add config_names as parameter to the method
  • document config_names in the docstring
  • keep configs as before
  • remove configs from the docstring
  • add a @_deprecate_arguments decorator to the method
  • handle both config_names and configs in the method logic. Basically do self.config_names = config_names or configs

Thanks in advance! 🤗

Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i am not sure this code was really used tbh... and we are changing the dataset repos on the Hub anyways)

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fine with changing it without deprecation warning then.

src/huggingface_hub/repocard_data.py Show resolved Hide resolved
@Wauplin Wauplin merged commit ee71a81 into huggingface:main Jun 2, 2023
@Wauplin
Copy link
Contributor

Wauplin commented Jun 2, 2023

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants