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

CI: Support for Python 3.9 #4301

Merged
merged 4 commits into from
Nov 5, 2020
Merged

CI: Support for Python 3.9 #4301

merged 4 commits into from
Nov 5, 2020

Conversation

csadorf
Copy link
Contributor

@csadorf csadorf commented Aug 14, 2020

According to the Python 3.9 RC release notes:

We strongly encourage maintainers of third-party Python projects to prepare their projects for 3.9 compatibility during this phase. As always, report any issues to the Python bug tracker.

I will track preparations to support Python 3.9 on this branch. The CI workflow for this branch is expected to fail until further notice.

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #4301 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4301      +/-   ##
===========================================
+ Coverage    79.49%   79.50%   +0.01%     
===========================================
  Files          482      482              
  Lines        35327    35327              
===========================================
+ Hits         28080    28082       +2     
+ Misses        7247     7245       -2     
Flag Coverage Δ
django 73.67% <ø> (+0.01%) ⬆️
sqlalchemy 72.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/engine/daemon/client.py 73.57% <0.00%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06063e7...0ff1ebf. Read the comment docs.

@sphuber
Copy link
Contributor

sphuber commented Oct 5, 2020

Just to note that Python 3.9 was officially released today. Should probably wait for our dependencies to release compatible versions, but hopefully we can also soon add support

@csadorf
Copy link
Contributor Author

csadorf commented Oct 6, 2020

Just to note that Python 3.9 was officially released today. Should probably wait for our dependencies to release compatible versions, but hopefully we can also soon add support

I'll update this PR.

@csadorf csadorf changed the title CI: Prepare support for Python 3.9 CI: Support for Python 3.9 Oct 6, 2020
@csadorf
Copy link
Contributor Author

csadorf commented Oct 6, 2020

Blocked by actions/setup-python#148 .

@csadorf
Copy link
Contributor Author

csadorf commented Oct 7, 2020

Standard pip install succeeds now, installation with extras still fails. Python 3.9 is not available via conda channels yet, but should be shortly.

@csadorf csadorf added the topic/dependencies/constraint Issues related to dependency constraints that should be resolved. label Oct 9, 2020
@csadorf
Copy link
Contributor Author

csadorf commented Oct 9, 2020

A Python 3.9 conda environment can now be created via the conda-forge channel, but there are still many conflicts, likely because upstream packages are not available yet for Python 3.9.

@csadorf
Copy link
Contributor Author

csadorf commented Oct 12, 2020

The pip install with extras is currently blocked by: nucleic/kiwi#92

@sphuber
Copy link
Contributor

sphuber commented Oct 12, 2020

Just realized that all of this is also completely dependent on our own maintained dependencies releasing a Python 3.9 compatible version: reentry, kiwipy, plumpy and maybe some others I am now forgetting. I will start working on the latter two.

@csadorf
Copy link
Contributor Author

csadorf commented Oct 21, 2020

The conda install now passes, pip install with extensions still failing, because of kiwisolver: nucleic/kiwi#92

A release of a Python 3.9 compatible version is promised for by the end of the week.

@csadorf
Copy link
Contributor Author

csadorf commented Nov 2, 2020

The pip install with and without extras succeeds now, but the conda installation no longer passes (go figure)..

@sphuber
Copy link
Contributor

sphuber commented Nov 2, 2020

The pip install with and without extras succeeds now, but the conda installation no longer passes (go figure)..

I am bit surprised that it passes at all to be honest. We haven't released a Python 3.9 compatible version for plumpy nor kiwipy yet. Granted, it may just be that the 3.8 version happens to be 3.9 compatible, but shouldn't pip automatically check if the package is explicitly marked to be compatible with 3.9? Or apparently it will always just try to install it?

@chrisjsewell
Copy link
Member

chrisjsewell commented Nov 2, 2020

I am bit surprised that it passes at all to be honest. We haven't released a Python 3.9 compatible version for plumpy nor kiwipy yet. Granted, it may just be that the 3.8 version happens to be 3.9 compatible, but shouldn't pip automatically check if the package is explicitly marked to be compatible with 3.9? Or apparently it will always just try to install it?

Well thats because of your loose python pinnings 😜
https://github.com/aiidateam/plumpy/blob/87337f649fe2b03e7007301b36e2633b6918fc4b/setup.py#L30
https://github.com/aiidateam/kiwipy/blob/7f6f642dcc527733ea57604e72e192afa6c6fd97/setup.py#L32

@sphuber
Copy link
Contributor

sphuber commented Nov 2, 2020

I am bit surprised that it passes at all to be honest. We haven't released a Python 3.9 compatible version for plumpy nor kiwipy yet. Granted, it may just be that the 3.8 version happens to be 3.9 compatible, but shouldn't pip automatically check if the package is explicitly marked to be compatible with 3.9? Or apparently it will always just try to install it?

Well thats because of your loose python pinnings stuck_out_tongue_winking_eye
https://github.com/aiidateam/plumpy/blob/87337f649fe2b03e7007301b36e2633b6918fc4b/setup.py#L30
https://github.com/aiidateam/kiwipy/blob/7f6f642dcc527733ea57604e72e192afa6c6fd97/setup.py#L32

A well, fair enough. I forgot about that, I was thinking whether pip also looked at the 'Programming Language :: Python :: 3.8' classifiers, but I guess it doesn't limit based on that.

@chrisjsewell
Copy link
Member

chrisjsewell commented Nov 2, 2020

A well, fair enough. I forgot about that, I was thinking whether pip also looked at the 'Programming Language :: Python :: 3.8' classifiers, but I guess it doesn't limit based on that.

No, I've been guilty of it as well, but I guess you should really pin python_requires to the same as the classifiers denote

@chrisjsewell
Copy link
Member

same with this repo actually:

"python_requires": ">=3.6",

@sphuber
Copy link
Contributor

sphuber commented Nov 2, 2020

A well, fair enough. I forgot about that, I was thinking whether pip also looked at the 'Programming Language :: Python :: 3.8' classifiers, but I guess it doesn't limit based on that.

No, I've been guilty of it as well, but you should really pin python_requires to the same as the classifiers denote

Isn't there a tradeoff? Clearly by adding an upper limit, you don't run the risk that the package will break for a new Python release. The same would happen with an upper limit, there it would just not even install and so the end result is the same, that package cannot be used in the new version. However, the advantage of not adding the upper limit is that there is a chance (and it being minor versions, they should be forwards compatible and therefore guaranteed) that the existing package simply works with the new version. This means upstream does not have to wait for a release. Imagine if every library put an upper limit. Each environment would then have to wait for everything downstream to have released a new version before it could do anything, massively slowing down migration to new Python releases.

I have never thought or read about this so this is an honest question on what the "best" practice is here.

@chrisjsewell
Copy link
Member

I have never thought or read about this so this is an honest question on what the "best" practice is here.

Yeh neither have I really.
I guess TBF all v3.x should be back compatible, so there should never be anything that breaks when you test against a new major version

@csadorf
Copy link
Contributor Author

csadorf commented Nov 2, 2020

I think it depends a bit on the complexity of the library/application. In the case of aiida-core it might make sense to set an upper limit and only increase it once it has been properly tested for that newer version. However, I would probably advocate for the "opposite" approach and only explicitly limit if incompatibilities are found.

@csadorf csadorf force-pushed the ci/python-39 branch 2 times, most recently from 9efad20 to 60b1523 Compare November 4, 2020 13:42
@csadorf csadorf marked this pull request as ready for review November 4, 2020 17:27
@csadorf csadorf requested review from ltalirz, sphuber and chrisjsewell and removed request for ltalirz and sphuber November 4, 2020 17:28
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @csadorf . Mostly have a few questions, some of which are in the code. Another one is that I am a bit surprised the tests actually pass. I have been seeing deprecation warnings coming from reentry for a while which still uses collections.Mapping which is deprecated and no longer works ion 3.9 in favor of collections.abc.Mapping. I guess that part of the code is not actually used in our code paths. @ltalirz I also found that reentry is still on dropd's fork. Should we move this to aiidateam and start to add other core members as maintainers in addition to yourself?

.github/workflows/test-install.yml Show resolved Hide resolved
.github/workflows/test-install.yml Show resolved Hide resolved
@csadorf
Copy link
Contributor Author

csadorf commented Nov 5, 2020

@sphuber I addressed your comments.

@csadorf csadorf requested a review from sphuber November 5, 2020 09:31
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

looks good thanks

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Looking good, thanks @csadorf

@csadorf
Copy link
Contributor Author

csadorf commented Nov 5, 2020

Looking good, thanks @csadorf

Thx! I'll wait for all tests to pass and then squash-merge.

Edit: Sorry, just saw your comment above, please go ahead then.

@sphuber sphuber merged commit 2c0f9a9 into aiidateam:develop Nov 5, 2020
@sphuber sphuber deleted the ci/python-39 branch November 5, 2020 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/dependencies/constraint Issues related to dependency constraints that should be resolved. topic/dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants