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

Limit Pydantic to V1 in dependencies #24596

Merged
merged 2 commits into from
Jun 30, 2023
Merged

Limit Pydantic to V1 in dependencies #24596

merged 2 commits into from
Jun 30, 2023

Conversation

lig
Copy link
Contributor

@lig lig commented Jun 30, 2023

Pydantic is about to release V2 release which will break a lot of things. This change prevents transformers to be used with Pydantic V2 to avoid breaking things.

Also, see #24597

Pydantic is about to release V2 release which will break a lot of things. This change prevents `transformers` to be used with Pydantic V2 to avoid breaking things.
@amyeroberts
Copy link
Collaborator

Hi @lig, thanks for opening this PR!

Could you provide some more information about the kind of issues / breakages expected? I can only see pydantic used in one place in the library here, so thankfully impact is limited.

For the quality checks, you'll need to run make style at the top level of the repo and push any changes made.

cc @ydshieh

@ydshieh ydshieh self-assigned this Jun 30, 2023
@ydshieh
Copy link
Collaborator

ydshieh commented Jun 30, 2023

This issue about pydantic is real. We get errors when trying to build docker image in the push CI triggered by commit 299aafe.

@lig Thank you for this PR, it helps us a lot before the issue! I also add one more change quickly (for our CI).

@amyeroberts I am going to merge once @sgugger approves.

2023-06-30T20:07:31.9883431Z  > [19/19] RUN python3 -c "from deepspeed.launcher.runner import main":
2023-06-30T20:07:31.9883916Z 1.621     from deepspeed.runtime.zero.config import DeepSpeedZeroConfig
2023-06-30T20:07:31.9884613Z 1.621   File "/usr/local/lib/python3.8/dist-packages/deepspeed/runtime/zero/config.py", line 76, in <module>
2023-06-30T20:07:31.9885116Z 1.621     class DeepSpeedZeroConfig(DeepSpeedConfigModel):
2023-06-30T20:07:31.9885814Z 1.621   File "/usr/local/lib/python3.8/dist-packages/pydantic/_internal/_model_construction.py", line 171, in __new__
2023-06-30T20:07:31.9886256Z 1.621     set_model_fields(cls, bases, config_wrapper, types_namespace)
2023-06-30T20:07:31.9886812Z 1.621   File "/usr/local/lib/python3.8/dist-packages/pydantic/_internal/_model_construction.py", line 361, in set_model_fields
2023-06-30T20:07:31.9887329Z 1.621     fields, class_vars = collect_model_fields(cls, bases, config_wrapper, types_namespace, typevars_map=typevars_map)
2023-06-30T20:07:31.9888039Z 1.621   File "/usr/local/lib/python3.8/dist-packages/pydantic/_internal/_fields.py", line 112, in collect_model_fields
2023-06-30T20:07:31.9888950Z 1.621     raise NameError(f'Field "{ann_name}" has conflict with protected namespace "{protected_namespace}"')
2023-06-30T20:07:31.9889546Z 1.621 NameError: Field "model_persistence_threshold" has conflict with protected namespace "model_"

@ydshieh ydshieh requested a review from sgugger June 30, 2023 20:41
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 30, 2023

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

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

LGTM!

@lig
Copy link
Contributor Author

lig commented Jun 30, 2023

@amyeroberts answering your question. I've had a quick look and I can say that this https://github.com/huggingface/transformers/blob/78a2b19fc84ed55c65f4bf20a901edb7ceb73c5f/src/transformers/commands/serving.py#L73C1-L73C36 will break.

Instead of

    tokens_ids: Optional[List[int]]

it should read

    tokens_ids: Optional[List[int]] = None

There is no implicit default None in Pydantic V2 here.

Thankfully, bump-pydantic helps with that https://github.com/pydantic/bump-pydantic/#bp001-add-default-none-to-optionalt-uniont-none-and-any-fields

@ydshieh ydshieh merged commit d51aa48 into huggingface:main Jun 30, 2023
18 of 20 checks passed
@ydshieh ydshieh mentioned this pull request Jul 3, 2023
@lmmx lmmx mentioned this pull request Dec 10, 2023
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.

None yet

5 participants