aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
437 stars 193 forks source link

Release v2.6.3 #6604

Closed agoscinski closed 3 weeks ago

agoscinski commented 3 weeks ago

Still waiting if we merge PR https://github.com/aiidateam/aiida-core/pull/6600 before.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (support/2.6.x@5faff07). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/aiida/orm/utils/serialize.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## support/2.6.x #6604 +/- ## ================================================ Coverage ? 77.80% ================================================ Files ? 562 Lines ? 41846 Branches ? 0 ================================================ Hits ? 32554 Misses ? 9292 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sphuber commented 3 weeks ago

Still waiting if we merge PR #6600 before.

Don't think we want to wait for this right? It is not critical and it seems it mostly depends on upstream dependencies that we don't control. Except for circus where I guess I have to fix a release.

agoscinski commented 3 weeks ago

Don't think we want to wait for this right? It is not critical and it seems it mostly depends on upstream dependencies that we don't control. Except for circus where I guess I have to fix a release.

I agree with you that it is most likely it is only upstream dependencies, even though I would like to see the test run through. But by pinning the requirements, we are not compatible with Python 3.13. I am not sure how to deal generally with such a situation. I would have upper bounded the version in this release and made a new release once we have fixed the dependencies. But if you don't like it, we can just merge it like this. I think it is more important to bring the bugfixes out rather than finding the perfect solution for this.

I added the commit e9d94f1b2a9620ae3356c5331a3758f9192c9b3c that fixes the pre-commit error, but did not include it in the CHANGELOG since it is just devops stuff that should not be relevant for enduser. Please let me know if you disagree.

Also I added manually the commit 7f8d2ddc9b0400eb0d9ec0d05ac3037cd5bd43cd to downgrade the Scheduler API usage to the old one to not include a breaking API change in this minor release.

The Docker Images CI fails here, but we have fixed it on main @unkcpz do you think it is relevant for the release? Cherry-picking the right commits would be a bit more work.

sphuber commented 3 weeks ago

I agree with you that it is most likely it is only upstream dependencies, even though I would like to see the test run through. But by pinning the requirements, we are not compatible with Python 3.13. I am not sure how to deal generally with such a situation.

Generally we don't do anything, and neither do most packages in the Python ecosystem I believe. The main point is to add an explicit lower bound.

I would have upper bounded the version in this release and made a new release once we have fixed the dependencies. But if you don't like it, we can just merge it like this. I think it is more important to bring the bugfixes out rather than finding the perfect solution for this.

We could start with this practice. I guess what would make most sense is to then simply always have an upper limit and when we add support for Python 3.13, we simply update the limit to <3.14 etc.

I added the commit e9d94f1 that fixes the pre-commit error, but did not include it in the CHANGELOG since it is just devops stuff that should not be relevant for enduser. Please let me know if you disagree.

Perfectly fine. If there are bigger changes in devops, I tend to still add them because they can be useful for developers to quickly track down when something was changed, but for minor things like this there is no point really.

Also I added manually the commit 7f8d2dd to downgrade the Scheduler API usage to the old one to not include a breaking API change in this minor release.

Yes. I would even have incorporated that change simply directly in the cherrypicked commit because it is really part of it. But this is fine as well. No need to change

The Docker Images CI fails here, but we have fixed it on main @unkcpz do you think it is relevant for the release? Cherry-picking the right commits would be a bit more work.

If it is a lot of work, fine to skip. But if it can be fixed by one more cherry-picked commit, it may be useful.

agoscinski commented 3 weeks ago

Yes. I would even have incorporated that change simply directly in the cherrypicked commit because it is really part of it. But this is fine as well. No need to change

I squashed it wtih the cherry picked commit. I added the edits message after the Cherry-pick hash to make it easier identifiable that this is a changed commit.

If it is a lot of work, fine to skip. But if it can be fixed by one more cherry-picked commit, it may be useful.

I tried to include some devops fixes but it ends becoming adding more and more commits until the CI is happy. To Fix the current error in the CI I need to add c52ec6758a, that requires cba6e7c757, that is maybe breaking change, and more commits. I sadly don't have time to track this down this week. So I would release without it.

agoscinski commented 3 weeks ago

@sphuber should be fine. It is on pypi https://pypi.org/project/aiida-core/ and GitHub https://github.com/aiidateam/aiida-core/releases/tag/v2.6.3

I sadly forgot to update the date in the CHANGELOG :/ should have marked it in a TODO list. I think of a way to catch this in future releases. For now I wrote it in6o the wiki in bold.

I followed further the instructions on the wiki and made a merge commit, but it seems that we have stopped doing this recently looking at the past releases. I actually like a merge commit that marks the commits the commits that are included. The commit message of the merge commit and the release commit (one before the merge commit) could be improved for the next release, right now they are a bit repetitive.

sphuber commented 3 weeks ago

@sphuber should be fine. It is on pypi https://pypi.org/project/aiida-core/ and GitHub https://github.com/aiidateam/aiida-core/releases/tag/v2.6.3

Fantastic, thanks a lot for the work!

I sadly forgot to update the date in the CHANGELOG :/ should have marked it in a TODO list. I think of a way to catch this in future releases. For now I wrote it in6o the wiki in bold.

No biggy. Since we are not merging back the support branch to main, it would anyway be good to add a commit on top of main to update the CHANGELOG.md and you can correct the date there.

Also, maybe we want to update the version on main with a commit like this: https://github.com/aiidateam/aiida-core/commit/16b8fe4a0912ac36973fb82f14691723e72599a7 Simply update it to v2.6.3.post0. For me it is not necessary, because it won't be technically correct. But there is no way of doing it "correctly" if you have support branches/releases. Most important part is that the .post0 suffix is there, but it already is.

I followed further the instructions on the wiki and made a merge commit, but it seems that we have stopped doing this recently looking at the past releases. I actually like a merge commit that marks the commits the commits that are included. The commit message of the merge commit and the release commit (one before the merge commit) could be improved for the next release, right now they are a bit repetitive.

This approach of a merge commit to mark the release only really works for the support branch though where we are creating a release branch with cherry-picked commits that is then merged. But if we will release directly from main, for example as we will do for v2.7.0 at some point in the near future, there won't be a branch to merge. There will just be the release commit that updates the changelog and version number.

danielhollas commented 1 week ago

No biggy. Since we are not merging back the support branch to main, it would anyway be good to add a commit on top of main to update the CHANGELOG.md and you can correct the date there.

@agoscinski would be good to update the CHANGELOG on main, since that's where people will generally look (at least that's what I do anyway).