Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

First mypy refactorings #1204

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rijkvandermeulen
Copy link
Contributor

@rijkvandermeulen rijkvandermeulen commented Sep 11, 2022

Current situation

I did a first investigation into the existing mypy errors and the amount of refactoring that would be required. Running mypy on the whole directory results in over 650 "errors". Some of these errors are super easy to fix, others will probably require slightly more effort.

Proposal

Curious to learn how you guys look at this, but in my opinion fixing >650 errors is quite a substantial effort for one PR. I'm therefore wondering whether it would make more sense to split this up into more "manageable" chunks? The way of working would then look something like:

  • I've created a mypy config file (mypy.ini) where we a.o. ignore the mypy errors of the part of the repo that we did not refactor yet.
  • This allows us to already add mypy to the pre-commit hooks as I did as part of this PR (since it will only check the part of the repo that we actually expect to comply with mypy).
  • Having this set-up in place, we can create seperate tickets to address individual parts of the repo (e.g., we refactor darts/timeseries/py and after that we remove: [mypy-darts.timeseries.*] ignore_errors = True. This would allow us to step by step improve.

To make all of this more practical I've already took a shot on resolving the mypy issues for darts/dataprocessing/dtw and darts/dataprocessing/pipeline.py. As you'll see in the mypy.ini these directories/files are thus excluded from the "exception list" and hence mypy will check these files after each commit.

Looking forward to hearing your thoughts on this approach!

@codecov-commenter
Copy link

Codecov Report

Base: 93.74% // Head: 93.72% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (057297a) compared to base (28ca88d).
Patch coverage: 96.77% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1204      +/-   ##
==========================================
- Coverage   93.74%   93.72%   -0.03%     
==========================================
  Files          83       83              
  Lines        8407     8396      -11     
==========================================
- Hits         7881     7869      -12     
- Misses        526      527       +1     
Impacted Files Coverage Δ
darts/dataprocessing/dtw/window.py 85.71% <93.75%> (-0.37%) ⬇️
darts/dataprocessing/dtw/cost_matrix.py 67.03% <100.00%> (ø)
darts/dataprocessing/dtw/dtw.py 94.16% <100.00%> (-0.05%) ⬇️
darts/dataprocessing/pipeline.py 86.84% <100.00%> (ø)
darts/timeseries.py 92.23% <0.00%> (-0.07%) ⬇️
...arts/models/forecasting/torch_forecasting_model.py 87.45% <0.00%> (-0.05%) ⬇️
darts/models/forecasting/block_rnn_model.py 98.24% <0.00%> (-0.04%) ⬇️
darts/models/forecasting/nhits.py 99.27% <0.00%> (-0.01%) ⬇️
darts/datasets/__init__.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hrzn
Copy link
Contributor

hrzn commented Sep 15, 2022

Thanks @rijkvandermeulen. It looks like a good way to do it. We'll try to review it sometime soon (we are a bit behind these days, apologies). We also will have to think whether the benefits (better type checks) over-weight the small cost (tracking all folders and dependencies in the ignore list, maintain the pre-commit hook). In any case I like your step-by-step approach!

Copy link
Contributor

@Borda Borda left a comment

Choose a reason for hiding this comment

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

just would be curious what is the runtime on whole project run --all-files

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