List mypy fix #6635

unkcpz · 2024-11-23T23:43:00Z

Separated from #6630, changes are 0b6f37f to solve the mypy error:

src/aiida/orm/nodes/data/list.py:84: error: Signature of "pop" incompatible with supertype "MutableSequence"  [override]
src/aiida/orm/nodes/data/list.py:84: note:      Superclass:
src/aiida/orm/nodes/data/list.py:84: note:          def pop(self, index: int = ...) -> Any
src/aiida/orm/nodes/data/list.py:84: note:      Subclass:
src/aiida/orm/nodes/data/list.py:84: note:          def pop(self, **kwargs: Any) -> Any
src/aiida/orm/nodes/data/list.py:92: error: Signature of "index" incompatible with supertype "Sequence"  [override]
src/aiida/orm/nodes/data/list.py:92: note:      Superclass:
src/aiida/orm/nodes/data/list.py:92: note:          def index(self, value: Any, start: int = ..., stop: int = ...) -> int
src/aiida/orm/nodes/data/list.py:92: note:      Subclass:
src/aiida/orm/nodes/data/list.py:92: note:          def index(self, value: Any) -> Any

and

src/aiida/tools/pytest_fixtures/daemon.py:130: error: Right operand of "and" is never evaluated  [unreachable]
src/aiida/tools/pytest_fixtures/daemon.py:131: error: Statement is unreachable  [unreachable]

May fix doc failed build

codecov · 2024-11-24T00:10:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.90%. Comparing base (ef60b66) to head (c0cc484).
Report is 142 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6635      +/-   ##
==========================================
+ Coverage   77.51%   77.90%   +0.40%     
==========================================
  Files         560      567       +7     
  Lines       41444    42148     +704     
==========================================
+ Hits        32120    32832     +712     
+ Misses       9324     9316       -8

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

agoscinski · 2024-11-25T11:12:28Z

src/aiida/common/pydantic.py

@@ -41,7 +41,7 @@ class Model(BaseModel):
    field_info = Field(default, **kwargs)

    for key, value in (('priority', priority), ('short_name', short_name), ('option_cls', option_cls)):
-        if value is not None:
+        if value is not None and field_info is not None:
            field_info.metadata.append({key: value})


I see the error but it seems strange. The Field constructor can return None?

just saw #6630 (comment) interesting

agoscinski · 2024-11-25T11:16:58Z

Looks good to me

src/aiida/tools/pytest_fixtures/daemon.py

danielhollas · 2024-11-25T23:27:13Z

src/aiida/orm/nodes/data/list.py

@@ -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:


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

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

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

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

@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?

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.

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?

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

danielhollas

Looks good. Thank you very much for following through with this and I am sorry for my misunderstanding yesterday.

unkcpz · 2024-11-26T13:37:26Z

@danielhollas no worries at all, thanks for the discussion, it was always very helpful.

unkcpz added 8 commits November 22, 2024 02:08

Bump mypy version to ~=1.13.0

3e4f3ef

ignore

b6800dc

Try fix

f415f55

Pydantic version caused mypy error

0dabade

Merge branch 'main' into mypy/fix/xx

2c0948f

amend

ac5473e

May fix doc failed build

rd

7b9e02e

Follow the abc contract of MutableSequence for orm.List

0b6f37f

unkcpz requested a review from danielhollas November 23, 2024 23:44

unkcpz mentioned this pull request Nov 25, 2024

Bump mypy version to ~=1.13.0 #6630

Merged

agoscinski reviewed Nov 25, 2024

View reviewed changes

agoscinski mentioned this pull request Nov 25, 2024

Bump ruff version #6614

Merged

Merge branch 'main' into list-mypy-fix

9ee616f

unkcpz commented Nov 25, 2024

View reviewed changes

src/aiida/tools/pytest_fixtures/daemon.py Show resolved Hide resolved

danielhollas reviewed Nov 25, 2024

View reviewed changes

Merge branch 'main' into list-mypy-fix

c0cc484

unkcpz requested a review from danielhollas November 26, 2024 09:51

danielhollas approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'main' into list-mypy-fix

715d4c2

unkcpz merged commit 36eab77 into aiidateam:main Nov 26, 2024
7 of 8 checks passed

unkcpz deleted the list-mypy-fix branch November 26, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List mypy fix #6635

List mypy fix #6635

unkcpz commented Nov 23, 2024 •

edited by danielhollas

Loading

codecov bot commented Nov 24, 2024 •

edited

Loading

agoscinski Nov 25, 2024 •

edited

Loading

agoscinski Nov 25, 2024

agoscinski commented Nov 25, 2024

danielhollas Nov 25, 2024

unkcpz Nov 26, 2024

danielhollas Nov 26, 2024

unkcpz Nov 26, 2024

sphuber Nov 26, 2024

unkcpz Nov 27, 2024 •

edited

Loading

sphuber Nov 27, 2024

unkcpz Nov 27, 2024

danielhollas left a comment

unkcpz commented Nov 26, 2024

List mypy fix #6635

List mypy fix #6635

Conversation

unkcpz commented Nov 23, 2024 • edited by danielhollas Loading

codecov bot commented Nov 24, 2024 • edited Loading

Codecov Report

agoscinski Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas left a comment

Choose a reason for hiding this comment

unkcpz commented Nov 26, 2024

unkcpz commented Nov 23, 2024 •

edited by danielhollas

Loading

codecov bot commented Nov 24, 2024 •

edited

Loading

agoscinski Nov 25, 2024 •

edited

Loading

unkcpz Nov 27, 2024 •

edited

Loading