-
Notifications
You must be signed in to change notification settings - Fork 115
Enable strict mypy settings #581
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #581 +/- ##
=======================================
- Coverage 98.2% 98.1% -0.1%
=======================================
Files 50 51 +1
Lines 6063 6260 +197
=======================================
+ Hits 5955 6143 +188
- Misses 108 117 +9
🚀 New features to boost your workflow:
|
10e097b to
a976b91
Compare
|
By now, this PR does a few other things, too:
Somehow, even after trying extensively to update my local venv to the same versions of packages as pre-commit/mypy uses, I couldn't reproduce some of the errors locally that mypy reports here (though running |
|
Do we need to edit the release notes for this one? |
e86bcfd to
1443df7
Compare
|
I've made some changes on the branch, and will try to summarize here before merging. Most of them are consequent of principles that can be roughly expressed as:
For instance, the fact that The added commits try to make changes where possible in line with those principles, while refraining from making big changes to internal and external APIs.
Also some improvements:
|
1443df7 to
b54920c
Compare
b54920c to
dfebb09
Compare
- Avoid immediately breaking message-ix code on `main`. - Add a warning that the method is deprecated.
…for use downstream in message-ix etc.
- Remove TYPE_CHECKING block, since the module should only itself be
imported within such a block.
- Add 7 types: Filters, ParData, ScalarParData, ScalarSolutionData,
SetData, SimpleSetData, SolutionData
- Rename ScenarioInfo → ScenarioIdentifiers to:
(a) make clear these (and not (model name, scenario name) alone)
uniquely identify a scenario
(b) avoid possible name clash with message_ix_models.ScenarioInfo.
- Rename ScenarioIdentifiers → ModelScenario.
- Rename ItemTypeFlags → ModelItemType.
- Loosen typing of InitializeItemsKwargs.ix_type.
- Assign flag values using auto(). - Remove short aliases like ItemType.S. - Add .name property that is always str, never None. - Add is_model_data() TypeGuard.
- Import from ixmp.types only when TYPE_CHECKING. - Use str instead of ItemTypeNames for 3 methods: - init_item() - item_get_elements() - list_items() - Adjust JDBCBackend and IXMP4Backend to match.
- Simplify construction of URLs. - Remove deprecated include_groups=… kwarg to DataFrameGroupBy.apply(). - Remove a type ignore for older pandas-stubs.
- Use a nested function, itertools.repeat(), and simpler logic. - Ensure expected dtypes in return values. - Improve comments, docstring. - Extend tests, note limitations of IXMP4Backend.
- Use tuples in CLASS_FOR_IX_TYPE; adjust/simplify throughout. - Handle scalars in methods - init_item(). - item_set_elements(). - item_get_elements(). - Add .ixmp4.Options.init_units to collect logic. - Fix incorrect use of log.warning() in discard_changes().
- Remove JDBCBackend-only mark. - Simplify test_util.test_diff_data(): - Use a for: block and TimeSeries.transact(). - Ensure data are sorted before modifying by index.
dfebb09 to
73116e3
Compare
|
Rebased after dealing with #586; I cherry picked two commits from this branch to ensure the RTD build succeed there. |
| @genno.config.handles("filters", iterate=False) | ||
| def filters(c: Computer, filters: dict): | ||
| # TODO Remove once genno adds annotation | ||
| @genno.config.handles("filters", iterate=False) # type: ignore[misc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you intending to open an issue upstream to request this? Should I?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did :)
|
|
||
|
|
||
| def store_ts(scenario, *data, strict: bool = False) -> None: | ||
| # NOTE Not sure why mypy can't import from pyam here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of 3.0.0, pyam-iamc is distributed without a py.typed marker file (see the distributions), so mypy considers the entire package to be untyped.
This is also why we list it in pyproject.toml with tool.mypy.overrides.ignore_missing_imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, pyam never had this marker file (at least since v2.0.0). I think I was still wondering why it only showed up in this one spot.
If I find a bit of time, I may try adding a py.typed file to pyam to resolve this :)
- Add docstrings to all types. - Rename WriteFiltersKwargs → WriteFilters (a) to align with Filters and (b) because these are usually not handled as distinct keyword args. - Rename ScenarioIdentifiers → TimeSeriesIdentifiers, since these refer to attributes of the TimeSeries class. - Remove 2 unused types: TSReadFileKwargs, SReadFileKwargs. - Adjust usage of these types.
- Add ixmp.types, ixmp.util.ixmp4 to API documentation. - Add migration notes.
Definitely yes. The added type hints may trigger new errors in downstream packages that use |
- Adjust hints in .core and .models. - Ignore missing type hints for pooch. - Remove outdated ignores for jpype, memory_profiler.
- Adjust hints in .core and .models. - Ignore missing type hints for pooch. - Remove outdated ignores for jpype, memory_profiler.
| "container_data": [], | ||
| } | ||
|
|
||
| def __init__(self, name_=None, **model_options): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the removal of a positional argument here broke the entire message_ix test suite; see iiasa/message_ix#963. I'll make another PR to revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#587.
Temporary addition between the merge of iiasa/ixmp#581 and the next release.
This PR enables settings that are equivalent to
mypy --strictand even slightly more :)There is still room for improvement and some cleanups, I think, but all tests should be passing already.
There are some dependencies that are not fully annotated (e.g. pint, dask, and genno) so that we have to implore some
type: ignores. These feel a bit unnecessary, especially when the underlying functions are documented clearly. For example,genno.Computer.get()is documented, but not typed, so we could think of proxying it here (essentially just calling thesuper()method) to include type hints (I also looked at genno if I could quickly provide the required type hints there, but got lost by more and more type hints and aborted the project to save some time).Other than that, I had to change several occurrences of
scen.par()in the test suite. Perpar()'s docstring (and function logic), it returns apd.DataFramefor indexed Parameters and adict[str, float | str]for scalar Parameters. Unfortunately, it was previously annotated to returnpd.DataFrameonly, which I changed now to the actual return type union. Several places in the test suite expect only apd.DataFrameto be returned, though.I'm not sure what the best thing to do is, here. We could maybe come up with another function for scalar Parameters (to allow
scen.par()to return onlypd.DataFrame), but that also requires code adjustments. Alternatively, we could adjustbackend.item_get_elements()to always returnpd.DataFrameforItemType.PAR, even for Scalars. Based on existing usage,scen.par()` doesn't seem to be called often to return Scalar data.How to review
PR checklist
Add or expand tests;coverage checks both ✅