Skip to content

Conversation

@ParagEkbote
Copy link
Contributor

@ParagEkbote ParagEkbote commented Apr 15, 2025

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
Member

@gen740 Could you review this PR?

@ParagEkbote
Copy link
Contributor Author

Could you please review the changes?

cc: @gen740

@github-actions
Copy link

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
Contributor Author

Not stale

@ParagEkbote
Copy link
Contributor Author

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
Member

gen740 commented May 9, 2025

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
Contributor Author

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

Copy link
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants