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

More utils doc #25457

Merged
merged 5 commits into from
Aug 17, 2023
Merged

More utils doc #25457

merged 5 commits into from
Aug 17, 2023

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Aug 11, 2023

What does this PR do?

This continues the work of cleaning up the scripts used for quality checks, which comes with a couple of fixes mainly:

  • the instructions for releases in the setup.py weren't necessarily up to date, so fixing those.
  • the script that auto-generates the transformers metadata used by the Hub didn't give the right processing class for ImageProcessors.

@sgugger sgugger requested review from ydshieh and LysandreJik August 11, 2023 10:15
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 11, 2023

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

Copy link
Member

@LysandreJik LysandreJik 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 this @sgugger ! I appreciate the extensive doc.

As mentioned offline, would love to have @ydshieh take a look and give an external eye to documents such as the release doc.

LGTM!

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thanks as always!

setup.py Outdated

6. Add a tag in git to mark the release: "git tag v<VERSION> -m 'Adds tag v<VERSION> for pypi' "
5. Add a tag in git to mark the release: "git tag v<VERSION> -m 'Adds tag v<VERSION> for pypi' "
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be from the release branch, right? If so, the doc doesn't make this (super) clear.

@@ -12,11 +12,30 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""
Utility that checks the big table in the index.md of the doc and potentially updates it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would love the full (relative) path if you are OK with this suggestion.

@@ -12,7 +12,26 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""
Utility that checks the list of models that can be used for each in task (as defined in the task-specific pages of the
Copy link
Collaborator

Choose a reason for hiding this comment

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

the list of models that can be used for each in task (as defined in the task-specific pages of the doc)

might be more clear if we mention tip, like

the list of models used in the tips in the task-specific pages of the doc

utils/release.py Outdated
PATH_TO_EXAMPLES = "examples/"
# This maps a type of file to the patter to look for when searching where the version is defined, as well as the
Copy link
Collaborator

Choose a reason for hiding this comment

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

patter -> pattern

@@ -12,41 +12,84 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""
Utility that prepares the repository for releases (or patches) by updating all versions in the relevant places. It
also performs some post-release cleanup, by updating the links in the main README to respective model doc pages (from
Copy link
Collaborator

Choose a reason for hiding this comment

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

when I read this, I am thinking this is only for the post-release. But after reading later stuff below, it seems to me the changes will (and should) be on the release branch and the main branch (after the release).

I am not strong for a change here, but if you happen to have a better wording, it would be nice.

@@ -65,11 +94,16 @@ def sort_auto_mapping(fname, overwrite: bool = False):
if overwrite:
with open(fname, "w", encoding="utf-8") as f:
f.write("\n".join(new_lines))
elif "\n".join(new_lines) != content:
return True
return "\n".join(new_lines) != content
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change requires a change in the docstring for Returns

@@ -46,7 +45,7 @@
_re_identifier = re.compile(r'\s*\(\s*"(\S[^"]+)"')


def sort_auto_mapping(fname: str, overwrite: bool = False) -> Optional[bool]:
def sort_auto_mapping(fname: str, overwrite: bool = False) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the following, yes, the return signature should be changed too (good catch!)

    Returns:
        `Optional[bool]`: Returns `None` if `overwrite=True`. Otherwise returns `True` if the file has an auto-mapping
improperly sorted, `False` if the file is okay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually put back the optional and fixed the return so it's correct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good for me!

@sgugger sgugger merged commit 2defb6b into main Aug 17, 2023
@sgugger sgugger deleted the more_utils_doc branch August 17, 2023 05:58
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
* Document and clean more utils.

* More documentation and fixes

* Switch to Lysandre's token

* Address review comments

* Actually put else
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