Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Features/add pre commit #64

Merged
merged 23 commits into from
Apr 18, 2023
Merged

Features/add pre commit #64

merged 23 commits into from
Apr 18, 2023

Conversation

MaGering
Copy link
Collaborator

Fixes #16.

This PR represents the cleaned up version of PR #54.

Open ToDos:

  • Add mypy
  • Format digipipe/digipipe with black and isort.

Before merging into dev-branch, please make sure that

  • if data flow was adjusted: data pipeline run finishes successfully.
  • new and adjusted code is formated using black and isort.
  • the CHANGELOG.rst was updated.
  • the docs were updated.

@MaGering MaGering self-assigned this Mar 20, 2023
@MaGering MaGering mentioned this pull request Mar 20, 2023
6 tasks
@MaGering
Copy link
Collaborator Author

Ready for review @nesnoj!

Just a few comments:
The pre-commit checks run with pre-commit run -a all except the one with mypy. For the flake8 and pylint checks I had to ignore many errors: flake8, pylint. This should be picked up again in the long run. Should I create a separate issue for this @nesnoj?
Regarding mypy, we should consider taking it out as a check because I don't see the benefits yet. I get the following errors, which I couldn't solve even after extended research:

mypy.....................................................................Failed
- hook id: mypy
- exit code: 2

digipipe/store/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.store" and "digipipe.store"
Found 1 error in 1 file (errors prevented further checking)
digipipe/scripts/config.py:8: error: Library stubs not installed for "yaml"  [import]
digipipe/scripts/config.py:8: note: Hint: "python3 -m pip install types-PyYAML"
digipipe/scripts/config.py:8: note: (or run "mypy --install-types" to install all missing stub packages)
digipipe/scripts/config.py:8: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
digipipe/scripts/config.py: error: Source file found twice under different module names: "digipipe.digipipe.scripts.config" and "digipipe.scripts.config"
Found 2 errors in 1 file (errors prevented further checking)
digipipe/store/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.store" and "digipipe.store"
Found 1 error in 1 file (errors prevented further checking)
digipipe/scripts/datasets/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.scripts.datasets" and "digipipe.scripts.datasets"
Found 1 error in 1 file (errors prevented further checking)
digipipe/scripts/geo.py: error: Source file found twice under different module names: "digipipe.digipipe.scripts.geo" and "digipipe.scripts.geo"
Found 1 error in 1 file (errors prevented further checking)
digipipe/store/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.store" and "digipipe.store"
Found 1 error in 1 file (errors prevented further checking)

@MaGering MaGering requested a review from nesnoj March 20, 2023 18:42
@MaGering MaGering mentioned this pull request Apr 13, 2023
4 tasks
Base automatically changed from features/switch_to_poetry to dev April 17, 2023 15:49
@nesnoj
Copy link
Member

nesnoj commented Apr 18, 2023

Ready for review @nesnoj!

Thank you, I did some basic tests and it looks good!
I fixed 2 minor issues in the install instructions.

Just a few comments: The pre-commit checks run with pre-commit run -a all except the one with mypy. For the flake8 and pylint checks I had to ignore many errors: flake8, pylint. This should be picked up again in the long run. Should I create a separate issue for this @nesnoj?

Yes, sounds reasonable 👍

Regarding mypy, we should consider taking it out as a check because I don't see the benefits yet. I get the following errors, which I couldn't solve even after extended research:

mypy.....................................................................Failed
- hook id: mypy
- exit code: 2

digipipe/store/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.store" and "digipipe.store"
Found 1 error in 1 file (errors prevented further checking)
digipipe/scripts/config.py:8: error: Library stubs not installed for "yaml"  [import]
digipipe/scripts/config.py:8: note: Hint: "python3 -m pip install types-PyYAML"
digipipe/scripts/config.py:8: note: (or run "mypy --install-types" to install all missing stub packages)
digipipe/scripts/config.py:8: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
digipipe/scripts/config.py: error: Source file found twice under different module names: "digipipe.digipipe.scripts.config" and "digipipe.scripts.config"
Found 2 errors in 1 file (errors prevented further checking)
digipipe/store/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.store" and "digipipe.store"
Found 1 error in 1 file (errors prevented further checking)
digipipe/scripts/datasets/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.scripts.datasets" and "digipipe.scripts.datasets"
Found 1 error in 1 file (errors prevented further checking)
digipipe/scripts/geo.py: error: Source file found twice under different module names: "digipipe.digipipe.scripts.geo" and "digipipe.scripts.geo"
Found 1 error in 1 file (errors prevented further checking)
digipipe/store/__init__.py: error: Source file found twice under different module names: "digipipe.digipipe.store" and "digipipe.store"
Found 1 error in 1 file (errors prevented further checking)
  • For the library stubs problem I found this explanation - apparently it can be at least suppressed. This is how @henhuy did it in the app. May be you want to give it a quick test but we can also kick mypy out if it takes too long to fix it.
  • I don't get the message "Source file found twice under different module names" but it might be caused by a buggy module structure: there's an __init__.py in digipipe/ (we don't need it here I think) but none in digipipe/digipipe/ (where it is actually needed). Could you please try to move the __init__.py to digipipe/digipipe/ and rerun mypy?

@MaGering
Copy link
Collaborator Author

The mypy check runs now after commenting out some of its error messages with commit d693760.

With commit 5b43f97 I moved the __init__.pyfile as you suggested to resolve the "Source file found twice under different module names" error.

@nesnoj
Copy link
Member

nesnoj commented Apr 18, 2023

👍

With commit 5b43f97 I moved the __init__.pyfile as you suggested to resolve the "Source file found twice under different module names" error.

And does it work?

@MaGering
Copy link
Collaborator Author

And does it work?

Yes! ;-) Tested the whole pipeline as well without running into errors.

Copy link
Member

@nesnoj nesnoj left a comment

Choose a reason for hiding this comment

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

Nice! :)

@MaGering MaGering merged commit 3899c4f into dev Apr 18, 2023
@MaGering MaGering deleted the features/add_pre-commit_ branch April 18, 2023 14:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pre-commit hooks
2 participants