Cleanup import statements: rearrange modules that _core imports from

[emabre]

DO NOT MERGE PLEASE

(Sorry, it seems I cannot add the corresponding do-not-merge label) This is a draft PR, and I just noticed some differences in the generated Python API docs. Fixes will follow and I will open the PR for review. Thanks.

Fixes #870 (Cleanup import statements(https://github.com/PowerGridModel/power-grid-model/issues/870))

Changes proposed in this PR include:

I split the files error.py, typing.py, data_types.py, enum.py by separating them in one part (located in _core/*) that includes the symbols that are imported by _core/* modules and another part that exposes the symbols to end user (to maintain backwards compatibility) and that includes other previously existing symbols that were not imported by core modules.

The results is that now modules under power_grid_model._core don't import back from modules in power_grid_model (e.g. but rather from modules power_grid_model._core.*.

This is how the new package diagram looks like after my change (the package diagram in the documentation is not colorized):

Could you please pay extra attention to the points below when reviewing the PR:

I changed and added some docstrings (I was required to add docstrings to new modules by the pylint hook, and I slightly edited the docstrings of the previously existing modules). In those docstrings I did not mention that the symbols defined therein may be imported by other modules and they might be visible to users because of that. My assumption is that the docstring of a module should not refer to what other modules would do with it, otherwise it's easy to forget to update it when the behavior of such other modules is changed. However, I wonder if those docstrings are clear enough. Modules where I added/edited the docstring:

• _core.errors
• data_types
• _core.data_types
• typing
• _core.typing

Tests that I have run

I ran the dir building Python function on the exposition modules where I applied functional changes, under src/power_grid_model (error.py, typing.py, data_types.py, enum.py). I checked that the list of symbols returned by that function does not miss any relevant symbol w.r.t. what was returned by calling that function over those modules in the main branch.

    import power_grid_model.data_types as data_types 
    import power_grid_model.errors as errors
    import power_grid_model.typing as typing
    import power_grid_model.enum as enum
    import power_grid_model.validation as validation
    import power_grid_model.utils as utils

    dt = dir(data_types)
    er = dir(errors)
    ty = dir(typing)
    en = dir(enum)
    va = dir(validation)
    ut = dir(utils)

# Then I performed the checks ...

[mgovers]

Hi @emabre , thank you once more for your contribution! I added the do-not-merge label for you. I also linked this PR to close #870 . Take all the time you need, your input is very much appreciated.

[emabre]

@mgovers thanks for helping. I am trying to fix the fact that sphinx's autodoc does not document symbols that are imported from other modules. At the same time I have some issues with local configs that prevent me from building the package locally. As soon as I am done I will push fixes to this branch.

[mgovers]

Hi @emabre ,

There is a merge conflict between this branch and main that requires manual resolution. More specifically, a new error type was implemented in https://github.com/PowerGridModel/power-grid-model/pull/947/files#diff-c67d705a0759f8037c1414e05e8f5f87ec3c3c0ae600e0f3b0bde92b75597826 . I think it should be fairly easy to resolve, but if needed, just LMK and I can help you out.

[emabre]

@mgovers thanks for letting me know. Yes, it shouldn't be difficult to reconcile my branch with main once I have fixed the issue that I found.

[emabre]

@mgovers thanks for your review, I will incorporate the changes soon. I am currently out for sprint break, with limited access to my PC, but I will try my best.

Actually, the main reason why this PR is stalled is that it seems not trivial (or not possible at all) to have the imported type aliases and module attributes documented properly when their docstrings reside in the module where those symbols are implemented. Everything works fine for classes and functions but not for simple data and types (it seems to be to the fact that "Python has no built-in support for docstrings for module data members or class attributes" https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html). I manage to get them show up in the docs, but their docstrings is automatically generated at best, it is not the one that was specified where they are implemented.

I assume that moving the docstrings out of the py files (i.e. document in the md files directly) is not an acceptable solution, since we have lots of symbols to document.

I have posted a question on stack overflow some hours ago about that https://stackoverflow.com/q/79586506/23534352

[mgovers]

@mgovers thanks for your review, I will incorporate the changes soon. I am currently out for sprint break, with limited access to my PC, but I will try my best.

Actually, the main reason why this PR is stalled is that it seems not trivial (or not possible at all) to have the imported type aliases and module attributes documented properly when their docstrings reside in the module where those symbols are implemented. Everything works fine for classes and functions but not for simple data and types (it seems to be to the fact that "Python has no built-in support for docstrings for module data members or class attributes" https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html). I manage to get them show up in the docs, but their docstrings is automatically generated at best, it is not the one that was specified where they are implemented.

I assume that moving the docstrings out of the py files (i.e. document in the md files directly) is not an acceptable solution, since we have lots of symbols to document.

I have posted a question on stack overflow some hours ago about that https://stackoverflow.com/q/79586506/23534352

Hi @emabre,

Thanks for your reply. Sounds like a hassle. If needed, we can so a solution like

from a import B as _B

B = _B
"""
Docstring for B
"""

It would be unfortunate but still better than what we currently have in main.

Let's await your stackoverflow post.

[emabre]

@mgovers it seems that the answers / comments to my post demonstrate that there is no elegant solution to the problem of auto-documenting module attributes, including type aliases. I am implementing the option that you proposed above. I am not far from finishing all the fixes, I need to double check a few things and then the PR will be ready

[mgovers]

@mgovers it seems that the answers / comments to my post demonstrate that there is no elegant solution to the problem of auto-documenting module attributes, including type aliases. I am implementing the option that you proposed above. I am not far from finishing all the fixes, I need to double check a few things and then the PR will be ready

Great! Thank you for the extensive investigation, it'll also be useful for future reference. Just let me know when I can review again and it should be good to go in a couple. Please note: due to potential merge conflicts with #720 , we may need to figure out a good moment to do the merge. @figueroa1395 and/or @nitbharambe will take over if need be since I'll be out of office for a bit.

[emabre]

Great! Thanks a lot for the assistance @mgovers I have completed the last checks and the PR is now ready for review. No problems for the conflicts with https://github.com/PowerGridModel/power-grid-model/issues/720, I'll wait for input by the people you mentioned earlier, if needed. Thanks

[mgovers]

#720 was merged; removing do-not-merge label

@emabre , Can you please resolve any possible merge conflicts (if any) and also check if everything still is as intended and no new issues have been created? I think everything should be fine but it's good to double-check since both PRs are big

[emabre]

Yes, unfortunately... I could experiment a bit more, but I won't be able to do it today (perhaps this evening, with some luck). So, if in the meantime others have some suggestions they are very much welcome 😁. Thanks

Il gio 1 mag 2025, 09:35 Martijn Govers @.***> ha scritto:

@.**** commented on this pull request.

In docs/api_reference/python-api-reference.md https://github.com/PowerGridModel/power-grid-model/pull/953#discussion_r2069938014 :

@@ -32,6 +32,8 @@ are internal implementations.

.. automodule:: power_grid_model.enum
   :undoc-members:
+   :imported-members:
+   :exclude-members: IntEnum

i see that bysource didn't work as you expected. Will check with the rest
of the team

—
Reply to this email directly, view it on GitHub
<https://github.com/PowerGridModel/power-grid-model/pull/953#discussion_r2069938014>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGCNSUBSE4LKENR5XIZDSST24HFC5AVCNFSM6AAAAAB3CBO4ACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMBZGI4DMOBSGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>

[emabre]

To add more context: I think the problem lies in how the symbols are sorted in the source code. However, inferring some kind order from the source code is not trivial when some items are imported and some others are directly implemented inside the module. On top of this, we are not totally free of choosing the order of the symbols in the source, because of the action of isort. I was doing some experimentations to try to better understand the mechanism but I could not come to a meaningful conclusion yet.

Il gio 1 mag 2025, 09:48 Ema B @.***> ha scritto:

Yes, unfortunately... I could experiment a bit more, but I won't be able to do it today (perhaps this evening, with some luck). So, if in the meantime others have some suggestions they are very much welcome 😁. Thanks

Il gio 1 mag 2025, 09:35 Martijn Govers @.***> ha scritto:

@.**** commented on this pull request.

In docs/api_reference/python-api-reference.md https://github.com/PowerGridModel/power-grid-model/pull/953#discussion_r2069938014 :

@@ -32,6 +32,8 @@ are internal implementations.

.. automodule:: power_grid_model.enum
   :undoc-members:
+   :imported-members:
+   :exclude-members: IntEnum

i see that bysource didn't work as you expected. Will check with the
rest of the team

—
Reply to this email directly, view it on GitHub
<https://github.com/PowerGridModel/power-grid-model/pull/953#discussion_r2069938014>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGCNSUBSE4LKENR5XIZDSST24HFC5AVCNFSM6AAAAAB3CBO4ACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMBZGI4DMOBSGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>

[mgovers]

To add more context: I think the problem lies in how the symbols are sorted in the source code. However, inferring some kind order from the source code is not trivial when some items are imported and some others are directly implemented inside the module. On top of this, we are not totally free of choosing the order of the symbols in the source, because of the action of isort. I was doing some experimentations to try to better understand the mechanism but I could not come to a meaningful conclusion yet. Il gio 1 mag 2025, 09:48 Ema B @.> ha scritto: … Yes, unfortunately... I could experiment a bit more, but I won't be able to do it today (perhaps this evening, with some luck). So, if in the meantime others have some suggestions they are very much welcome 😁. Thanks Il gio 1 mag 2025, 09:35 Martijn Govers @.> ha scritto: > @.**** commented on this pull request. > ------------------------------ > > In docs/api_reference/python-api-reference.md > <#953 (comment)> > : > > > @@ -32,6 +32,8 @@ are internal implementations. > ```{eval-rst} > .. automodule:: power_grid_model.enum > :undoc-members: > + :imported-members: > + :exclude-members: IntEnum > > i see that bysource didn't work as you expected. Will check with the > rest of the team > > — > Reply to this email directly, view it on GitHub > <#953 (comment)>, > or unsubscribe > https://github.com/notifications/unsubscribe-auth/AGCNSUBSE4LKENR5XIZDSST24HFC5AVCNFSM6AAAAAB3CBO4ACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMBZGI4DMOBSGE > . > You are receiving this because you were mentioned.Message ID: > @.***> >

@emabre , we had a chat with the PGM maintainers and we decided that alphabetical order is fine. I think this PR is in a great state as is. We will thoroughly review with 2 pairs of eyes (because it contains some quite sensitive stuff that's easy to get wrong) and will merge if all looks good.

[emabre]

Hi Martijn, thanks again for the great news! Please review carefully that all the symbols are there in the docs with their docstrings. I had checked personally all of them but I did not repeat the verification after my latest pushes yesterday evening. I did not do so because the sorting problem was still open and I planned to review them again once it was finished. (I don't expect issues though)

Il gio 1 mag 2025, 12:52 Martijn Govers @.***> ha scritto:

mgovers left a comment (PowerGridModel/power-grid-model#953) https://github.com/PowerGridModel/power-grid-model/pull/953#issuecomment-2844636866

To add more context: I think the problem lies in how the symbols are sorted in the source code. However, inferring some kind order from the source code is not trivial when some items are imported and some others are directly implemented inside the module. On top of this, we are not totally free of choosing the order of the symbols in the source, because of the action of isort. I was doing some experimentations to try to better understand the mechanism but I could not come to a meaningful conclusion yet. Il gio 1 mag 2025, 09:48 Ema B @.

> ha scritto: … <#m_1937657340393532715_> Yes, unfortunately... I could experiment a bit more, but I won't be able to do it today (perhaps this evening, with some luck). So, if in the meantime others have some suggestions they are very much welcome 😁. Thanks Il gio 1 mag 2025, 09:35 Martijn Govers @.> ha scritto: > @.**** commented on this pull request. > ------------------------------ > > In docs/api_reference/python-api-reference.md > <#953 (comment) https://github.com/PowerGridModel/power-grid-model/pull/953#discussion_r2069938014>

: > > > @@ -32,6 +32,8 @@ are internal implementations. > ```{eval-rst} > .. automodule:: power_grid_model.enum > :undoc-members: > + :imported-members: > + :exclude-members: IntEnum > > i see that bysource didn't work as you expected. Will check with the > rest of the team > > — > Reply to this email directly, view it on GitHub > <#953 (comment) https://github.com/PowerGridModel/power-grid-model/pull/953#discussion_r2069938014>, or unsubscribe > https://github.com/notifications/unsubscribe-auth/AGCNSUBSE4LKENR5XIZDSST24HFC5AVCNFSM6AAAAAB3CBO4ACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMBZGI4DMOBSGE . > You are receiving this because you were mentioned.Message ID: > @.***>

@emabre https://github.com/emabre , we had a chat with the PGM maintainers and we decided that alphabetical order is fine. I think this PR is in a great state as is. We will thoroughly review with 2 pairs of eyes (because it contains some quite sensitive stuff that's easy to get wrong) and will merge if all looks good.

— Reply to this email directly, view it on GitHub https://github.com/PowerGridModel/power-grid-model/pull/953#issuecomment-2844636866, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGCNSUCBBTKZKU2EIG35CN324H4FHAVCNFSM6AAAAAB3CBO4ACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBUGYZTMOBWGY . You are receiving this because you were mentioned.Message ID: @.***>

[emabre]

Thanks for the careful review. I must have excluded ComponentAttributeFilterOptions because it was already available in enum, but I did not consider that it should also be available in typing, sorry for that. I applied the fix that was requested.

[emabre]

I force-pushed because I needed to move the import of ComponentAttributeFilterOptions a couple of rows below as requested by isort