Skip to content

Conversation

@djsaunde
Copy link
Collaborator

@djsaunde djsaunde commented Oct 8, 2025

This PR adds the ruff (check and format) pre-commit hook in order to get this codebase closer to PEP 8 compliance. It also adds a GitHub action that runs on PRs / pushes to main that runs the pre-commit hooks.

This ensures any PRs that are incoming have code that is formatted properly, so we don't have to have code style discussion as part of PR review.

Note that the only "manual" changes are (1) adding the pre-commit hooks / configuration, and (2) adding the GitHub action to run the hooks; the rest of the changes come from applying the ruff pre-commit hook.

Some more complicated error codes are ignored and can be handled in future PRs.

@djsaunde
Copy link
Collaborator Author

I added scripts/enforce_kwargs_spacing.py and scripts/run_ruff_format.py to keep our custom foo = bar kwargs spacing intact. This PR should be good to go!

@djsaunde
Copy link
Collaborator Author

Lmk if / when we'd like to merge and I can resolve the merge conflicts.

past_key_value: Optional[Tuple[torch.Tensor]] = None,
output_attentions: bool = False,
use_cache: bool = False,
padding_mask: Optional[torch.LongTensor] = None,
Copy link
Collaborator

@rolandtannous rolandtannous Nov 2, 2025

Choose a reason for hiding this comment

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

fine by me. this is pythonic but
@danielhanchen to validate if ok for him in terms of alignment of key:value pairs vs original . I doubt this can be automated though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh the spacings? Was gonna ask it, but it's fine for now

@rolandtannous
Copy link
Collaborator

LGTM

@danielhanchen to confirm:

  • do we keep pass statements in codebase
  • variable alignments (tagged on example) however this can't be automated.

@danielhanchen
Copy link
Contributor

@djsaunde LGTM! :) I like the extra spacings as well!

@djsaunde djsaunde force-pushed the ruff branch 2 times, most recently from f5647d0 to 0d806da Compare November 3, 2025 01:49
@djsaunde
Copy link
Collaborator Author

djsaunde commented Nov 3, 2025

@danielhanchen I fixed the merge conflicts, can you merge if this is all set?

@danielhanchen danielhanchen changed the base branch from main to nightly November 3, 2025 02:26
@danielhanchen
Copy link
Contributor

@djsaunde I'm rebasing this to nightly - can you fix merge conflicts thanks

@djsaunde
Copy link
Collaborator Author

djsaunde commented Nov 3, 2025

@danielhanchen all set.

@danielhanchen danielhanchen merged commit 2dcb6d5 into unslothai:nightly Nov 5, 2025
1 check passed
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