Skip to content

[docstring] Fix docstring for AltCLIPTextConfig, AltCLIPVisionConfig and AltCLIPConfig#27128

Merged
amyeroberts merged 7 commits intohuggingface:mainfrom
AksharGoyal:contributing-docstring
Oct 31, 2023
Merged

[docstring] Fix docstring for AltCLIPTextConfig, AltCLIPVisionConfig and AltCLIPConfig#27128
amyeroberts merged 7 commits intohuggingface:mainfrom
AksharGoyal:contributing-docstring

Conversation

@AksharGoyal
Copy link
Contributor

@AksharGoyal AksharGoyal commented Oct 29, 2023 •
edited
Loading

What does this PR do?

  • Fixed docstrings for AltCLIPTextConfig, AltCLIPVisionConfig and AltCLIPConfig
  • Cleaned few docstrings

Fixes #26638

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

amyeroberts
amyeroberts approved these changes Oct 29, 2023
View reviewed changes
Copy link
Contributor

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for adding all these fixes!

Just a small comment. Once resolved we're good to merge 🤗

utils/check_docstrings.py Outdated
"AlignTextModel",
"AlignVisionConfig",
"AltCLIPTextConfig",
# "AltCLIPTextConfig",
Copy link
Contributor

@amyeroberts amyeroberts Oct 29, 2023

Choose a reason for hiding this comment

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

This should just be removed rather than commented out

Suggested change
# "AltCLIPTextConfig",

Copy link
Contributor Author

@AksharGoyal AksharGoyal Oct 29, 2023

Choose a reason for hiding this comment

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

Ok I will remove that. I guess I can do the same for VisionConfig too as they were in the same file.

@AksharGoyal
Copy link
Contributor Author

AksharGoyal commented Oct 29, 2023 •
edited
Loading

Hi @amyeroberts I have made the change. Let me know if anything else is needed from my side. For some reason removing AltCLIPVisionConfig is giving errors so I let it stay there. Any tip on resolving it would be helpful.

@amyeroberts
Copy link
Contributor

amyeroberts commented Oct 29, 2023

@AksharGoyal Thanks! For the removal of the AltCLIPVisionConfig what errors are you getting?

@AksharGoyal
Copy link
Contributor Author

AksharGoyal commented Oct 29, 2023

@amyeroberts This is what I see when I go to one of the failed workflows

2023-10-29 19:05:35.437483: W tensorflow/compiler/tf2tensorrt/utils/py_utils.cc:38] TF-TRT Warning: Could not find TensorRT
/home/circleci/transformers/src/transformers/deepspeed.py:23: FutureWarning: transformers.deepspeed module is deprecated and will be removed in a future version. Please import deepspeed modules directly from transformers.integrations
  warnings.warn(
Traceback (most recent call last):
  File "utils/check_docstrings.py", line 1231, in <module>
    check_docstrings(overwrite=args.fix_and_overwrite)
  File "utils/check_docstrings.py", line 1223, in check_docstrings
    raise ValueError(error_message)
ValueError: There was at least one problem when checking docstrings of public objects.
The following objects docstrings contain templates you need to fix: search for `<fill_type>` or `<fill_docstring>`.
- AltCLIPVisionConfig

Exited with code exit status 1

@amyeroberts
Copy link
Contributor

amyeroberts commented Oct 30, 2023

@AksharGoyal The issue is arising because some of the parameters in AltCLIPVisionConfig's docstring aren't correctly filled in. You'll see now in the configuration file <fill_docstring> has been added in places where descriptions are needed

amyeroberts
amyeroberts approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for iterating!

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 31, 2023

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@amyeroberts amyeroberts merged commit 9234cae into huggingface:main Oct 31, 2023
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
@AksharGoyal @EduardoPach
[docstring] Fix docstring for AltCLIPTextConfig, AltCLIPVisionConfig …
2fe7759 
…and AltCLIPConfig (huggingface#27128)

* [docstring] Fix docstring for AltCLIPVisionConfig, AltCLIPTextConfig + cleaned some docstring

* Removed entries from check_docstring.py

* Removed entries from check_docstring.py

* Removed entry from check_docstring.py

* [docstring] Fix docstring for AltCLIPTextConfig, AltCLIPVisionConfig and AltCLIPConfig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@amyeroberts amyeroberts amyeroberts approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Community Event] Docstring Sprint

3 participants

@AksharGoyal @amyeroberts @HuggingFaceDocBuilderDev