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

Add pyproject.toml #690

Merged
merged 3 commits into from
Sep 1, 2023
Merged

Add pyproject.toml #690

merged 3 commits into from
Sep 1, 2023

Conversation

mnoukhov
Copy link
Contributor

Other huggingface projects such as transformers, peft, and accelerate all have pyproject.toml files that allow different editors to standardize formatting during editing. Although there is already a pre-commit setup.cfg it becomes a bit annoying when the style is overridden during editing.

I've added a simple default pyproject.toml mostly mimicking peft's and keeping the same parameters as the setup.cfg. Happy to add / remove any parameters.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 24, 2023

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

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks, I left one comment. cc @lvwerra @vwxyzjn

pyproject.toml Outdated
@@ -0,0 +1,37 @@
[tool.black]
line-length = 119
target-version = ['py37']
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be py38 as we recently dropped 3.7 support #441

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for catching that

@vwxyzjn
Copy link
Contributor

vwxyzjn commented Aug 25, 2023

This change makes sense to me!

Copy link
Contributor

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks! Left some comments and suggestions to keep us aligned with other libraries (namely accelerate as an example)

pyproject.toml Outdated
target-version = ['py38']

[tool.ruff]
ignore = ["C901", "E501", "E741", "W605"]
Copy link
Contributor

Choose a reason for hiding this comment

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

At least in Accelerate, we never enforce E501. If we want to keep it consistent across frameworks, adding suggestions for doing so

Suggested change
ignore = ["C901", "E501", "E741", "W605"]
# Never enforce `E501` (line length violations).
ignore = ["C901", "E741", "W605"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit confused. You're suggesting to remove "E501" from the ignore list, so wouldn't it then be enforced? Accelerate includes it in the ignore list.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right! I misread, thanks

pyproject.toml Outdated

[tool.ruff]
ignore = ["C901", "E501", "E741", "W605"]
select = ["C", "E", "F", "I", "W"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select = ["C", "E", "F", "I", "W"]
select = ["E", "F", "I", "W"]

pyproject.toml Outdated
lines-after-imports = 2
known-first-party = ["trl"]

[isort]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer isort to be just left in the setup.cfg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that it wouldn't sort work correctly with the line length without this block, but it seems to be fine! Removing

pyproject.toml Outdated
Comment on lines 32 to 37
[tool.pytest]
doctest_optionflags = [
"NORMALIZE_WHITESPACE",
"ELLIPSIS",
"NUMBER",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does trl do docstring as part of their testing suite? In accelerate we do not, it only makes sense here if trl does this cc @younesbelkada

Copy link
Contributor

@younesbelkada younesbelkada Aug 28, 2023

Choose a reason for hiding this comment

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

thanks a lot @muellerzr , indeed we do not test docstrings in our testing suite, so we can maybe safely remove that section

@mnoukhov
Copy link
Contributor Author

After suggestions, I made the file equivalent to accelerate's

Copy link
Contributor

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks a bunch!

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thank you!

@lvwerra lvwerra merged commit 9f69f06 into huggingface:main Sep 1, 2023
11 checks passed
kushal-tri pushed a commit to kushalarora/trl that referenced this pull request Sep 19, 2023
* example pyproject.toml

* update target to py38

* make pyproject.toml equivalent to accelerate
lapp0 pushed a commit to lapp0/trl that referenced this pull request May 10, 2024
* example pyproject.toml

* update target to py38

* make pyproject.toml equivalent to accelerate
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.

6 participants