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

use isinstance instead of type comparison #26748

Closed
wants to merge 10 commits into from

Conversation

kashif
Copy link
Contributor

@kashif kashif commented Oct 12, 2023

What does this PR do?

Fix for E721 errors which are annoying when doing make style with a newer ruff

@HuggingFaceDocBuilderDev

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

Copy link
Collaborator

@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 all these changes to improve the code!

Mostly just small nits. Only main comment is the modification of the pinned ruff versions.

@@ -20,7 +20,7 @@
import os
import re
import sys
import time
from time import sleep
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personal opinion, but I think the previous version was better. Using time.sleep makes it more obvious which functionality and library is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right the issue was that there is a function time defined in the Message class which gives a "Redefinition" warning...

src/transformers/dependency_versions_table.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@kashif
Copy link
Contributor Author

kashif commented Oct 30, 2023

@amyeroberts thanks! so I relaxed the ruff version as with these changes the latest ruff runs without raising any issues... but yes i un-did that as per your request

Copy link
Collaborator

@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 these improvements to the code!

@kashif
Copy link
Contributor Author

kashif commented Nov 16, 2023

fixed by #27144

@kashif kashif closed this Nov 16, 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.

3 participants