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

Fix ixmp4 tests and support Python 3.12 #820

Closed
wants to merge 9 commits into from

Conversation

glatterf42
Copy link
Collaborator

@glatterf42 glatterf42 commented Feb 29, 2024

Please confirm that this PR has done the following:

  • Tests Fixed
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • Description in RELEASE_NOTES.md Added

Description of PR

FYI @meksor, @phackstock, @danielhuppmann.

This PR fixes the test failure reported in iiasa/ixmp4#50 by configuring dask to not convert None to string[pyarrow_numpy]. This is following the suggested workaround in dask/dask#10631 (comment), which is not a sustainable solution, which might be provided by dask/dask-expr#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
Copy link
Collaborator Author

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

@glatterf42
Copy link
Collaborator Author

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

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

@danielhuppmann
Copy link
Member

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

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

@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 Add extra check if both values are NA in bulk_upsert 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
Copy link
Collaborator Author

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

Closing in favor of #821

@glatterf42
Copy link
Collaborator Author

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.

@glatterf42 glatterf42 mentioned this pull request Mar 1, 2024
4 tasks
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.

Pyam does currently not support 3.12
3 participants