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

Support new Python versions #73

Merged
merged 29 commits into from
Nov 18, 2024
Merged

Support new Python versions #73

merged 29 commits into from
Nov 18, 2024

Conversation

danielhollas
Copy link
Collaborator

@danielhollas danielhollas commented Nov 17, 2024

Test with new Python versions up to 3.12 (aiida-core doesn't support 3.13 yet).

EDIT: See comments below, in this PR I restricted the aiida-core versions to below or equal to 2.2, which is the last version that doesn't break any tests. Will open a follow-up PR.

@danielhollas
Copy link
Collaborator Author

danielhollas commented Nov 17, 2024

The archive_cache tests are failing for AiiDA 2.6, which changed which objects are included in the hash:

I'll investigate more tomorrow.

import aiida_testing

load_documentation_profile()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was removed since it is no longer needed in AiiDA 2.0

Copy link
Member

Choose a reason for hiding this comment

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

@@ -37,7 +30,6 @@
with contextlib.suppress(ImportError):
import sphinx_rtd_theme
html_theme = 'sphinx_rtd_theme'
html_theme_path = [sphinx_rtd_theme.get_html_theme_path()]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently no longer necessary, generated a warning

include:
#Test for aiida-core 1.X (since aiida-core 2.X does not support python 3.7)
- python-version: '3.7'
editable_install_option: '-e'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's just test non-editable install here, we install editably in pre-commit and docs jobs anyway.

@danielhollas danielhollas changed the title CI updates Support new Python and AiiDA versions Nov 17, 2024
@danielhollas
Copy link
Collaborator Author

Unbelievable, all green for AiiDA 2.2. So version 2.3 broke the (ironically named) test_broken_code

FAILED mock_code/test_diff.py::test_broken_code - AssertionError: assert () == ('1,2c1', '< ...silly walks.')
  
  Right contains 5 more items, first extra item: '1,2c1'
  
  Full diff:
  + ()
  - (
  -     '1,2c1',
  -     '< Lorem ipsum dolor..',
  -     '<',
  -     '---',
  -     '> Please report to the ministry of silly walks.',
  - )
=================== 1 failed, 19 passed, 1 warning in 36.57s ===================
Error: Process completed with exit code 1.
0s
0s

I am putting this up for review, and will do a follow up fixes for 2.3 and 2.6 in separate PR so that the fixes are not drowned in unrelated changes here.

@danielhollas danielhollas marked this pull request as ready for review November 18, 2024 00:06
@@ -55,20 +59,13 @@ testing = [
]
pre_commit = [
"pre-commit",
"pylint~=2.12.2",
"mypy==0.930",
"pylint~=3.3.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm gonna get rid of pylint for ruff, but in a separate PR.

Copy link
Collaborator Author

@danielhollas danielhollas Nov 18, 2024

Choose a reason for hiding this comment

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

I begin to really appreciate the beauty of single rust binaries, this pylint pin was a reason why the installation for newer Python versions did not work. :-(

"pre-commit",
"pylint~=2.12.2",
"mypy==0.930",
'aiida-testing[testing,pre_commit,docs]',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Self-referencing extras are allowed in pyproject.toml (but not in setup.cfg)

@danielhollas danielhollas changed the title Support new Python and AiiDA versions Support new Python versions Nov 18, 2024
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @danielhollas to look at this repo.
I remember now there was a discussion on the naming of the package at aiidateam/aiida-registry#206

If we anyway want to change the name and make a release, I think we can start to support it from the latest version (or the nearest one) ?

#Test for aiida-core 1.X (since aiida-core 2.X does not support python 3.7)
- python-version: '3.7'
editable_install_option: '-e'
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12']
Copy link
Member

Choose a reason for hiding this comment

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

We can ask on the discourse and if no one using aiida-core < 2 with this anymore, maybe can drop the support for 3.7 and 3.8?
I think if it is going to make a release, it is not a problem to drop the support and versioning it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's have this discussion at #72. There I proposed to release the code basically as is (after renaming it) as version 0.1.0 so that existing users can easily switch to the official package. After that all breaking changes and droping support can be done in v0.2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

one using aiida-core < 2 with this anymore, maybe can drop the support for 3.7 and 3.8?

btw: Dropping support for AiiDA v1 should anyway be done in a separate PR, since there will be a lot of code cleanup associated with that.

import aiida_testing

load_documentation_profile()
Copy link
Member

Choose a reason for hiding this comment

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

@danielhollas
Copy link
Collaborator Author

@unkcpz thanks for taking a look!

I'd like this PR to be only about making the CI green and supporting new Python version.

Regarding naming, the full discussion is in #60. Let's discuss release in #72.
Regarding dropping AiiDA v1 support, I'll do it in a separate PR since that's going to be larger cleanup.

@unkcpz
Copy link
Member

unkcpz commented Nov 18, 2024

I'd like this PR to be only about making the CI green and supporting new Python version.

I see, and I agree small PRs are betters. Would you mind open an issue and put those plans with checkbox? I guess you'll working on this during the coding week?

@danielhollas
Copy link
Collaborator Author

Yep, the plan is to work on this during coding week. I'll open an issue with a plan.

@danielhollas
Copy link
Collaborator Author

Before I merge this I'll try to fix the tests for newer AiiDA versions. If it is not too hard I'll do it in this PR.

@danielhollas
Copy link
Collaborator Author

I'll merge this, opened #75 as follow up for tomorrow.

@danielhollas danielhollas merged commit fdd0de1 into main Nov 18, 2024
32 checks passed
@danielhollas danielhollas deleted the drop-aiidav1 branch November 18, 2024 17:16
@danielhollas danielhollas mentioned this pull request Nov 19, 2024
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.

2 participants