Skip to content

Conversation

@SamuelMarks
Copy link
Collaborator

@SamuelMarks SamuelMarks commented Jul 20, 2025

Description

Many of my contributions have been to improve the overall quality of this codebase through application of automated code-quality tooling. You already have pre-commit hooks; this PR adds everything in your code_style.sh into it; so no longer will people be able to contribute code so low quality it doesn't pass the automated code-quality checking tooling.

Along the way I found and fixed actual errors, like in MaxText/convert_deepseek_unscanned_ckpt.py when the API changed (#1837) and tests didn't pickup the issue but pytype did. Ditto for the required MaxText/sft_trainer.py update from #1327. Also some incorrect types being used (ints vs strings; ints vs np.ndarray; floats vs np.ndarray) and add missing __init__.pys so imports work properly (no pytype module-error).

EDIT: Following internal team-member request, I wrote and executed this script to split this PR up. Don't close this PR, but merge the linked ones first.

fd --exact-depth 2 -Ftd -j1 -x bash -c 'd=${0:2}; d=${d//\//.}; title="[${d}] pytype + pylint + pyink + codespell"; git switch main && git reset --hard && git checkout -b "qa_${d}" && git checkout auto_check_and_precommit_and_bash_code_style_sh "${0}" && git commit --no-verify -S -am "${title}" && git push mine "qa_${d}" ; gh pr create --title "${title}" --body "$(cat -- /tmp/a.md)"' {//}

Tests

N/A

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed.

@SamuelMarks SamuelMarks force-pushed the auto_check_and_precommit_and_bash_code_style_sh branch 2 times, most recently from 2105218 to 2581d6c Compare September 19, 2025 21:55
@SamuelMarks SamuelMarks force-pushed the auto_check_and_precommit_and_bash_code_style_sh branch from 2581d6c to 5eb5592 Compare September 27, 2025 14:14
@SamuelMarks SamuelMarks force-pushed the auto_check_and_precommit_and_bash_code_style_sh branch 2 times, most recently from 7d08cc1 to 3701642 Compare September 27, 2025 17:32
@SamuelMarks SamuelMarks force-pushed the auto_check_and_precommit_and_bash_code_style_sh branch 2 times, most recently from aea9fb7 to c98c291 Compare September 29, 2025 16:16
@SamuelMarks SamuelMarks requested a review from xuefgu as a code owner September 29, 2025 16:16
Copy link
Collaborator

@richjames0 richjames0 left a comment

Choose a reason for hiding this comment

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

lgtm pending gemini review. it wasn't working so i just removed and re-added the tag. PTAL before submitting

@SamuelMarks SamuelMarks force-pushed the auto_check_and_precommit_and_bash_code_style_sh branch from c98c291 to 7f23bac Compare September 29, 2025 19:55
…mmit-hooks.yaml,.editorconfig] Init ; [*] Modify code to be compliant with pre-commit hooks
@SamuelMarks SamuelMarks force-pushed the auto_check_and_precommit_and_bash_code_style_sh branch from 7f23bac to a9c4867 Compare September 29, 2025 19:56
Copy link
Collaborator

@NicoGrande NicoGrande left a comment

Choose a reason for hiding this comment

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

LGTM

@RissyRan
Copy link
Collaborator

RissyRan commented Sep 30, 2025

@richjames0 & @shralex I think I know the reason now.

This PR is forked version from SamuelMarks:auto_check_and_precommit_and_bash_code_style_sh, and the workflow for gemini-review has rule github.event.pull_request.head.repo.fork == false. This rule is recommended to protect our resources from forked branches (out of control for usage).

@copybara-service copybara-service bot merged commit 9033e75 into AI-Hypercomputer:main Oct 1, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants