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

List mypy fix #6635

Merged
merged 11 commits into from
Nov 26, 2024
Merged
7 changes: 4 additions & 3 deletions src/aiida/orm/nodes/data/list.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"""`Data` sub class to represent a list."""

from collections.abc import MutableSequence
from typing import Any

from .base import to_aiida_type
from .data import Data
Expand Down Expand Up @@ -81,15 +82,15 @@ def remove(self, value):
self.set_list(data)
return item

def pop(self, **kwargs): # type: ignore[override]
def pop(self, index: int = -1) -> Any:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did a little history digging, and the **kwargs implementation dates all the way back 8 years ago when the List class was first implemented.

b509727#diff-d02f48db7287263f108efca9214eac06220796c1ba81696adb6764852629f71dR259

I don't know if that means anything, but this change still looks quite scary to me, given that we're changing a public method on a such a basic type. 😰

Copy link
Member Author

Choose a reason for hiding this comment

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

I think these two methods are not directly called but to mimic the regular list datatype behavior, therefore we can keep it synchronous?
If we cannot come up with approach to further investigate how this change will break other things, we can add it to the next minor version change (I don't think we will bump the major in the near future, minor version can contain some "maybe backward compatibility" changes like this I assume)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, definitely should be documented in the CHANGELOG just in case. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!
I never did the release of aiida-core, but I think it might be hard to remember what need to be put in the CHANGELOG. I don't know how it was done now, but maybe it will be helpful if for some feature add or potential break changes like this one. In the PR we modify the CHANGELOG and putting things to the "nightly" release which is the main branch. For the real release, we can then just pick things from the nightly to the version.
What do you think? @agoscinski @sphuber

(we don't need to worry about this if there was not too much things happened in the code base, so it is not very urgent thing to have a very clear release plan I assume.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@unkcpz I simply had the info in my head. Since I was involved with pretty much all PRs, or would always look at commits made even if not directly involved, I would be aware of what changes were on main. When planning releases, I would go through the commits and write the changelog, marking which commits would contain breaking changes etc. Since I am not fully involved anymore, I think it makes sense to record these changes as we go along as to make it clearer for anyone. Your suggestion to include updates to the CHANGELOG with PRs makes sense to me.

In the PR we modify the CHANGELOG and putting things to the "nightly" release which is the main branch. For the real release, we can then just pick things from the nightly to the version.

What do you mean with the nightly release here?

Copy link
Member Author

@unkcpz unkcpz Nov 27, 2024

Choose a reason for hiding this comment

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

What in my mind was something like https://pypi.org/project/streamlit-nightly/ (also a lot other python libraries having this). It is build for every commit main (or we can make it build daily/weekly based with scheduled GH action). It is build and pushed to pypi by CI like https://github.com/streamlit/streamlit/blob/develop/.github/workflows/nightly.yml

For the changelog part, I was thinking for each PR we add a line in "## Nigthly" section on top of CHANGELOG and the CI will read it and use it as the release description for nightly release. If we decide to make real release, the CI will read it and use it as the release description for nightly release. If we decide to make real release, the items in the "nightly" section can be moved under the release version section and the CI will then pick it and update the description for each release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that would work for a simple branching workflow where everything goes on main and you always release directly from main. But that is not what we are doing. For example, there are a lot of changes on main now that we didn't want to release straight away so we could test it. The updated engine and scheduler interface for example. In the meantime we then make patch releases on the support/2.6.x branch. This takes some manual work and not sure how you can automate this. Besides that, do we really need this? If you look at the volume of commits that we have for the last few months, there is very little going on that would require automated releases I would say. And if someone really wants to run those changes, they can simply check out the repo and install from there?

Copy link
Member Author

Choose a reason for hiding this comment

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

there is very little going on that would require automated releases I would say. And if someone really wants to run those changes, they can simply check out the repo and install from there?

Yes, I think so. So probably the most convenient way is to put things in the CHANGELOG (or somewhere else just in case we didn't synchronous too much between multiple developers in the "MAGA" office).
Thanks for the suggestion @sphuber! I'll keep it as it is now and when we are about to make next release I'll discuss with @agoscinski

"""Remove and return item at index (default last)."""
data = self.get_list()
item = data.pop(**kwargs)
item = data.pop(index)
if not self._using_list_reference():
self.set_list(data)
return item

def index(self, value): # type: ignore[override]
def index(self, value: Any, start: int = 0, stop: int = 0) -> int:
"""Return first index of value.."""
return self.get_list().index(value)

Expand Down
2 changes: 1 addition & 1 deletion src/aiida/tools/pytest_fixtures/daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def test(submit_and_await):
from aiida.engine import ProcessState

def factory(
submittable: type[Process] | ProcessBuilder | ProcessNode | t.Any,
submittable: type[Process] | ProcessBuilder | ProcessNode,
unkcpz marked this conversation as resolved.
Show resolved Hide resolved
state: ProcessState = ProcessState.FINISHED,
timeout: int = 20,
**kwargs,
Expand Down
Loading