Skip to content

Setup Pre-Commit#316

Merged
gen740 merged 6 commits intooptuna:mainfrom
ParagEkbote:Setup-Precommit
May 15, 2025
Merged

Setup Pre-Commit#316
gen740 merged 6 commits intooptuna:mainfrom
ParagEkbote:Setup-Precommit

Conversation

@ParagEkbote
Copy link
Copy Markdown
Contributor

@ParagEkbote ParagEkbote commented Apr 15, 2025 •
edited
Loading

When I was working on #303, the pre-commit was not setup. This PR does the same. I have taken reference from this repo: https://github.com/optuna/optuna/blob/master/.pre-commit-config.yaml. Please let me know if any improvements are needed and I will make the necessary changes.

cc: @c-bata

@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented Apr 22, 2025

@gen740 Could you review this PR?

@ParagEkbote
Copy link
Copy Markdown
Contributor Author

ParagEkbote commented Apr 29, 2025

Could you please review the changes?

cc: @gen740

@github-actions
Copy link
Copy Markdown

github-actions bot commented May 6, 2025

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label May 6, 2025
@ParagEkbote
Copy link
Copy Markdown
Contributor Author

ParagEkbote commented May 7, 2025

Not stale

@ParagEkbote
Copy link
Copy Markdown
Contributor Author

ParagEkbote commented May 7, 2025

Could you please review the changes?

cc: @gen740

@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label May 7, 2025
@gen740
Copy link
Copy Markdown
Member

gen740 commented May 9, 2025 •
edited
Loading

I’m so sorry for my late reply, and thank you very much for your pull request!
(I was on a business trip…)

I ran the pre-commit hook on your branch, and the following error occurred:

> pre-commit run --all-files
black....................................................................Passed
flake8...................................................................Passed
isort....................................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 2

pytorch/pytorch_simple.py: error: Duplicate module named "pytorch_simple" (also at "multi_objective/pytorch_simple.py")
pytorch/pytorch_simple.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
pytorch/pytorch_simple.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)

Does this also happen on your machine?
I guess the pre-commit hook should run the same checks as defined in .github/workflows/checks.yml
Currently, that doesn’t seem to be the case, which means mypy checks aren’t guaranteed to run.

Also, it would be really helpful for beginners if you added instructions to CONTRIBUTING.md, just like Optuna does.
https://github.com/optuna/optuna/blob/022f97dbcb9be635ba2987662a33f684ad9811f0/CONTRIBUTING.md?plain=1#L81-L90

@ParagEkbote
Copy link
Copy Markdown
Contributor Author

ParagEkbote commented May 9, 2025

Since this repo doesn't include checks for mypy as seen in the config file here, I've removed it from the pre-commit file and now all the checks are passing.

image

Could you please review?

cc: @gen740

gen740
gen740 approved these changes May 15, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@gen740 gen740 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gen740 gen740 merged commit d54bf4f into optuna:main May 15, 2025
1 check passed
@nzw0301 nzw0301 added this to the v4.4.0 milestone May 16, 2025
@ParagEkbote ParagEkbote deleted the Setup-Precommit branch May 16, 2025 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gen740 gen740 gen740 approved these changes

Assignees

@gen740 gen740

Labels

None yet

Projects

None yet

Milestone

v4.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@ParagEkbote @HideakiImamura @gen740 @nzw0301