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

Release v2.6.3 #6604

Merged
merged 8 commits into from
Nov 6, 2024
Merged

Conversation

agoscinski
Copy link
Contributor

Still waiting if we merge PR #6600 before.

Copy link

codecov bot commented Nov 4, 2024

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 ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##             support/2.6.x    #6604   +/-   ##
================================================
  Coverage                 ?   77.80%           
================================================
  Files                    ?      562           
  Lines                    ?    41846           
  Branches                 ?        0           
================================================
  Hits                     ?    32554           
  Misses                   ?     9292           
  Partials                 ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sphuber
Copy link
Contributor

sphuber commented Nov 4, 2024

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

agoscinski commented Nov 5, 2024

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 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.

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.

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

sphuber commented Nov 5, 2024

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.

unkcpz and others added 8 commits November 5, 2024 14:14
For the `aiida-core-with-services` image where the services are part of
the image, we cannot rely on the health check for the services provided
by docker-build as is used for the `aiida-core-base` case. Instead, a
simple sleep was added to the `aiida-prepare.sh` script that sets up
the profile, to make sure the services are up before the profile is
created.

This solution is neither elegant nor robust. Here the sleep approach is
replaced by `s6-notifyoncheck`. This hook allows blocking the startup
from continuing until a script returns a 0 exit code. The script in
question first calls `rabbitmq-diagnostics ping` to make sure the
RabbitMQ server is even up, followed by a call to
`rabbitmq-diagnostics check_running`. If the latter returns 0, it means
RabbitMQ is up and running and the script returns 0 as well, which will
trigger `s6-notifyoncheck` to continue with the rest of the startup.

Note that `rabbitmq-diagnostics is_running` could not be used as that
command sometimes returns 0 even if the service is not ready at all.

Co-authored-by: Sebastiaan Huber <[email protected]>

Cherry-pick: 9579378
In e952d77 a bug in `verdi plugin list`
was fixed where the conditional to check whether the plugin was a
process class would always raise an `AttributeError` if the plugin was
not a `Process` or a proces function. As a result, the code would never
get to the else-clause.

The else-clause contained itself another bug, which was now revealed by
the fixing of the bug in the conditional. The else-clause would call the
`get_description` classmethod of the plugin, but no classes in AiiDA
that are pluginnable even define such a class method. Probably, the
original author confused it with the instance method `get_description`
but the `verdi plugin list` command just deals with the class.

The `get_description` call is replaced with just getting `__doc__` which
returns the docstring of the class/function, or `None` if it is not
defined. In the latter case, a default message is displayed saying that
no description is available.

Since the else-clause code was never reached before the recent fix and
the `get_description` method was never supported officially by AiiDA's
pluginnable interfaces, it is fine to just change this behavior.

Cherry-pick: c3b10b7
…ects (aiidateam#6566)

Apparently, somehow `mypy` is updated and it is complaining about an `ignore[assignment]` comment that previously was imposed by `mypy` itself. (in src/aiida/orm/utils/serialize.py::L51)
This commit removed `ignore[assignment]`, and `ci-style / pre-commit (pull_request)`  is passing now.

Cherry-pick: 655da5a
The current implementation only issues a kill command for the
parent process, but this can leave child processes orphaned. The
child processes are now retrieved and added explicitly to the
kill command.

Cherry-pick: fddffca

Edits: Downgrades scheduler usage to old API in fix aiidateam#6572

The fix in aiidateam#6572 was pushed after the `Scheduler` API has been
refactored in PR aiidateam#6043. To not include this breaking change in a minor
release we adapt the usage of `Scheduler` in the PR to the old API.
…version (aiidateam#6581)

The grep command failed that extracts the python version from mamba list failed because an indentation was added.
See the python string in the output of `mamba list`:

```
List of packages in environment: "/opt/conda"

  Name    Version  Build               Channel
────────────────────────────────────────────────────
  python  3.10.13  hd12c33a_1_cpython  conda-forge
```

Cherry-pick: 332a4a9
…ds not starting with aiida (aiidateam#6573)

With the version update of bake-action the `BAKE_METADATA` a field with warning
information (buildx.build.warnings) was added that does not contain the key
`image.name` so the json query failed. With this commit another json query was
added that filters out every field name that does not start with "aiida", so
the field with warning information is filtered out.

Co-authored-by: Jusong Yu <[email protected]>

Cherry-pick: e1467ed
…idateam#6580)

Processes start to broadcast their event before they update their
process status in the database. This can cause issues if the next
process directly tries to access the last process state retrieving it
from the database while it has not been updated in the database.

Cherry-pick: 867353c
@agoscinski
Copy link
Contributor Author

agoscinski commented Nov 5, 2024

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 c52ec67, that requires cba6e7c, 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.

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 @agoscinski Let's forget about the docker workflow and release this. Do you still have the time to do that? Are you familiar with the next steps for the release?

@agoscinski agoscinski merged commit cfd9ced into aiidateam:support/2.6.x Nov 6, 2024
10 of 11 checks passed
@agoscinski
Copy link
Contributor Author

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

sphuber commented Nov 6, 2024

@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: 16b8fe4 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
Copy link
Collaborator

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).

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.

5 participants