IAMconsortium / pyam

Analysis & visualization of energy & climate scenarios
https://pyam-iamc.readthedocs.io/
Apache License 2.0
220 stars 115 forks source link

Fix ixmp4 tests and support Python 3.12 #820

Closed glatterf42 closed 4 months ago

glatterf42 commented 4 months ago

Please confirm that this PR has done the following:

Description of PR

FYI @meksor, @phackstock, @danielhuppmann.

This PR fixes the test failure reported in https://github.com/iiasa/ixmp4/issues/50 by configuring dask to not convert None to string[pyarrow_numpy]. This is following the suggested workaround in https://github.com/dask/dask/issues/10631#issuecomment-1915417729, which is not a sustainable solution, which might be provided by https://github.com/dask-contrib/dask-expr/issues/691.

Coincidentally, I ran the tests in a local venv of Python 3.12 and they all seem to pass (after adapting a matplotlib option in one tutorial), so this PR might close #798. For that, the steps suggested in setup.cfg still have to be executed because of minimum version bumps and the release notes/docs have to be updated as suggested above.

@phackstock please let me know if you want me to finish the PR or if you want to take a look (since the python support might be non-trivial and a new release would then probably be warranted).

glatterf42 commented 4 months ago

I'm wondering about the pytest-legacy workflow: won't these lines install the legacy packages first, but then update them?

glatterf42 commented 4 months ago

Please note that while applying ruff to the code base, I didn't take the time to make all your functions compatible with the code complexity limit we set for ixmp4. Instead, please search for # noqa: C901 to discover which functions have been excluded from this measurement.

Also, I'd recommend using mypy to automatically detect issues like the following:

    def validate(
        self,
        criteria: dict = None,
        *,
        upper_bound: float = None,
        lower_bound: float = None,
        exclude_on_fail: bool = False,
        **kwargs,
    ) -> pd.DataFrame:

The type hints are off. Someone could think it's okay to not set e.g. criteria, but doing so would most likely result in a failure.

glatterf42 commented 4 months ago

Sorry, looks like I missed one notebook. Feel free to cancel the tests and restart.

danielhuppmann commented 4 months ago

I'm wondering about the pytest-legacy workflow: won't these lines install the legacy packages first, but then update them?

No, the second pip install will not upgrade existing packages if they are compatible with the package dependencies.

danielhuppmann commented 4 months ago

Re @glatterf42' comment about the validate() signature - the signature is correct and criteria is not a required argument, instead a user should use keyword arguments directly. The criteria argument is kept as a legacy-compatibility fix and will be removed in the next major release.

phackstock commented 4 months ago

@glatterf42, thanks for the quick updates on this PR. To me it currently combines three sets of changes that should be their own PRs:

  1. Use & apply ruff for formatting and linting
  2. Officially update pyam to support python 3.12
  3. Fix the failing ixmp4 test (that one should not be necessary anymore after iiasa/ixmp4#53)

So if you don't mind, I'd suggest that you close this PR and open two new ones for points 1. and 2. above. Once we have a new release of ixmp4 we can then remove the ixmp4 version pin to see if the tests run.

glatterf42 commented 4 months ago

As discussed just now, #821 is for ruff and we'll just remove the workaround for the failing test from here once ixmp4 releases 0.7.1, so no need for a third PR.

danielhuppmann commented 4 months ago

Closing in favor of #821

glatterf42 commented 4 months ago

Okay, then I'll open another PR with commits cherry-picked from here to support Python 3.12. Note to self: I also need to update the badge in index.rst.