Skip to content

fix(pybind): register bound types before the signatures that reference them - #654

Open
leonardocarreras wants to merge 1 commit into
fix/pybind-enum-arg-reprfrom
fix/pybind-registration-order
Open

leonardocarreras wants to merge 1 commit into
fix/pybind-enum-arg-reprfrom
fix/pybind-registration-order

Conversation

@leonardocarreras

Copy link
Copy Markdown
Contributor

pybind11 resolves a parameter's type when def() runs, so a class registered later in the module renders as its raw C++ name, for example CPS::IdentifiedObject, CPS::SystemTopology or CPS::SimNode<double>. pybind11-stubgen degrades those to ..., so the shipped stubs typed set_system, add_event, connect and connect_component as bare Any.

Declares every py::class_ handle up front, populates the submodules, then attaches the .def()s, and splits addSignalComponentBases() out of addSignalComponents() to break the signal to base to dp dependency cycle. Together with the pull request below this takes pybind11-stubgen from 129 errors to 0, verified by rebuilding the module both ways on the same toolchain.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

Found 13 medium (0 anchored to lines below).

🔵 Optional / low-confidence (13)
  • Missing documentation for new base class bindings [medium · 35% confidence · unconfirmed] in dpsim/src/pybind/SignalComponents.cpp:43
  • Add documentation page for TurbineGovernorType1 signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/TurbineGovernorType1.md
  • Add documentation page for GovernorParameters signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/GovernorParameters.md
  • Add documentation page for Governor signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/Governor.md
  • Add documentation page for TurbineParameters signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/TurbineParameters.md
  • Add documentation page for Turbine signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/Turbine.md
  • Add documentation page for PSSParameters signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/PSSParameters.md
  • Add documentation page for PSS signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/PSS.md
  • Missing test for addSignalComponentBases ordering [medium · 35% confidence · unconfirmed] in dpsim/src/pybind/SignalComponents.cpp:272
  • Missing documentation for newly bound PSS base class [medium · 30% confidence · unconfirmed] in dpsim/src/pybind/SignalComponents.cpp:43
  • Missing documentation for newly bound Governor/Turbine base classes [medium · 30% confidence · unconfirmed] in dpsim/src/pybind/SignalComponents.cpp:61
  • Missing documentation for newly bound signal base component classes [medium · 30% confidence · unconfirmed] in dpsim/src/pybind/SignalComponents.cpp:30
  • Missing test coverage for newly exposed signal base classes [medium · 30% confidence · unconfirmed] in dpsim/src/pybind/SignalComponents.cpp:36
Claim vs. implementation
  • Claimed: Register bound pybind types before any signatures that reference them, and split signal base registrations to break a dependency cycle.
  • Done: Reorders module/submodule and class registration in pybind main, predeclares several core types before attaching methods, and splits signal base class bindings into a new addSignalComponentBases() called before addSignalComponents().
  • Difference: none
How this review was produced

13 specialized finder passes raised 56 findings over the diff and the full changed sources. After de-duplication, 56 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 43 refuted as unsupported, 13 kept (13 tentative).

Refuted by verification:

  • Verify base class registration order for TurbineGovernorType1 (dpsim/src/pybind/SignalComponents.cpp): The file registers the base classes before the concrete TurbineGovernorType1 binding inside addSignalComponentBases, matching the intended pybind multiple-inheritance order.
  • Missing override for mnaComp hooks in TurbineGovernorType1 (dpsim/src/pybind/SignalComponents.cpp): TurbineGovernorType1 is a signal component and the file only binds set_parameters/initialize_states; no mnaComp hooks are present or required here.
  • Missing parameter validation in TurbineGovernorType1::setParameters (dpsim/src/pybind/SignalComponents.cpp): The binding simply forwards setParameters with named arguments; there is no validation logic in the binding to omit or add here.
  • Missing state initialization guard in TurbineGovernorType1::initializeStates (dpsim/src/pybind/SignalComponents.cpp): The initialize_states binding directly forwards initializeStates with the TmRef argument; no guard is present in the file.
  • Use DOUBLE_EPSILON for near-zero checks in governor/turbine parameter bindings (dpsim/src/pybind/SignalComponents.cpp): The file does not perform input validation or use DOUBLE_EPSILON in these bindings.
  • Ensure Signal component bases are registered before Signal components to avoid dependency cycle in task graph (dpsim/src/pybind/main.cpp): The file explicitly calls addSignalComponentBases(mSignal) before addSignalComponents(mSignal), matching the intended composite registration pattern.
  • Ensure all py::class_ handles remain alive until module definition completes (dpsim/src/pybind/main.cpp): The py::class_ handles are declared in the same PYBIND11_MODULE scope and remain live through the later .def() uses.
  • Missing override specifier on virtual destructor for Base classes (dpsim/src/pybind/SignalComponents.cpp): The related base headers already declare virtual destructors for GovernorParameters, Governor, PSSParameters, PSS, TurbineParameters, and Turbine.
  • TurbineGovernorType1 constructor exposes long positional parameter list (dpsim/src/pybind/SignalComponents.cpp): The constructor is intentionally exposed via py::init<std::string, CPS::Logger::Level>() in the binding.
  • Unused base class bindings in addSignalComponentBases (dpsim/src/pybind/SignalComponents.cpp): TopologicalSignalComp and SimSignalComp are used as base classes in later py::class_ registrations throughout the same file.
  • Rename function to reflect its purpose (dpsim/src/pybind/SignalComponents.cpp): The function name addSignalComponentBases accurately describes that it registers base signal classes before the concrete bindings.
  • Inconsistent submodule docstring (dpsim/src/pybind/main.cpp): The submodule is intentionally named with the docstring "signal models" at the m.def_submodule call.
  • Remove Logger::Level constructor from TurbineGovernorType1 binding (dpsim/src/pybind/SignalComponents.cpp): The Logger::Level constructor is explicitly bound for TurbineGovernorType1, so it is not missing or removable on this evidence.
  • Remove Logger::Level constructor from ExciterDC1 binding (dpsim/src/pybind/SignalComponents.cpp): ExciterDC1 also exposes a std::string, Logger::Level constructor in the same file.
  • Remove Logger::Level constructor from ExciterDC1Simp binding (dpsim/src/pybind/SignalComponents.cpp): ExciterDC1Simp also exposes a std::string, Logger::Level constructor in the same file.

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

TL;DR: The only issues surfaced are four medium documentation gaps for newly exposed signal components; the binding-order fix itself did not produce any correctness, scheduling, or equation/stamping concerns in the review passes. Overall risk is low, but the new Python-facing types should have matching docs to avoid discoverability and usage problems.

Found 4 medium (0 anchored to lines below).

🔵 Optional / low-confidence (4)
  • Add documentation page for TurbineGovernorType1 signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/TurbineGovernorType1.md
  • Add documentation page for PSS signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/PSS.md
  • Add documentation page for Governor signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/Governor.md
  • Add documentation page for Turbine signal component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Signal/Turbine.md
How this review was produced

13 specialized finder passes raised 47 findings over the diff and the full changed sources. After de-duplication, 47 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 43 refuted as unsupported, 4 kept (4 tentative).

Refuted by verification:

  • Missing base class registration for PSSParameters and PSS in addSignalComponentBases (dpsim/src/pybind/SignalComponents.cpp): PSSParameters and PSS are explicitly registered in addSignalComponentBases at lines 43-47
  • Add missing override for SimSignalComp in TurbineGovernorType1 bindings (dpsim/src/pybind/SignalComponents.cpp): TurbineGovernorType1 is bound with py::class_<..., CPS::SimSignalComp>(..., py::multiple_inheritance())
  • Missing external tag for side-effecting task registration in addSignalComponentBases (dpsim/src/pybind/main.cpp): The file only reorders binding calls; there is no task registration or Scheduler dependency code here, and the composite binding pattern is not implicated.
  • Order of addSignalComponents after addSignalComponentBases may still create dependency cycle in task graph (dpsim/src/pybind/main.cpp): The line simply calls addSignalComponents after addSignalComponentBases, matching the intended split and order in the file.
  • Ensure Simulation class is fully defined before RealTimeSimulation binding (dpsim/src/pybind/main.cpp): Simulation is fully bound before the RealTimeSimulation block begins, and RealTimeSimulation is declared as deriving from Simulation in the binding.
  • Use consistent py::class_ handle naming for base classes (dpsim/src/pybind/main.cpp): The base-class handles are declared and then used later in the file for method bindings and submodule setup.
  • Add missing override specifiers to new virtual hooks in Base classes (dpsim/src/pybind/SignalComponents.cpp): The related base headers already declare these methods virtual/pure virtual, so no override declaration issue is shown here
  • TurbineGovernorType1 exposes long positional parameter list in Python binding (dpsim/src/pybind/SignalComponents.cpp): This is a pybind argument list for an existing API, not a defect in the file
  • Use named argument for logLevelArg in Simulation constructor binding (dpsim/src/pybind/main.cpp): logLevelArg is used consistently as the named default helper in the constructor binding, not as an unnamed positional argument.
  • Use named argument for logLevelArg in RealTimeSimulation constructor binding (dpsim/src/pybind/main.cpp): RealTimeSimulation uses logLevelArg(CPS::Logger::Level::info) exactly at the constructor binding line.
  • Use named argument for logLevelArg in CIMReader constructor binding (dpsim/src/pybind/main.cpp): CIMReader uses logLevelArg for both defaults, including the named "comploglevel" argument.
  • Rename function to reflect its new purpose (dpsim/src/pybind/SignalComponents.cpp): The function name addSignalComponentBases matches the implementation and the header refactor shown in the diff
  • Add module docstring for submodule 'event' (dpsim/src/pybind/main.cpp): The event submodule is created with a docstring: m.def_submodule("event", "events").
  • Use SPDLOG_LOGGER_DEBUG for debug-level logging in constructor (dpsim/src/pybind/SignalComponents.cpp): The constructor binding itself is present; the logging-style preference is not a code defect in this file
  • Use SPDLOG_LOGGER_DEBUG for debug-level logging in ExciterDC1 constructor (dpsim/src/pybind/SignalComponents.cpp): The constructor binding itself is present; the logging-style preference is not a code defect in this file

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

No issues surfaced by the automated passes.

How this review was produced

13 specialized finder passes raised 27 findings over the diff and the full changed sources. After de-duplication, 27 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 27 refuted as unsupported, 0 kept (0 tentative).

Refuted by verification:

  • Verify base class registration order matches inheritance hierarchy (dpsim/src/pybind/SignalComponents.cpp): The file explicitly registers the base classes before the derived bindings in addSignalComponentBases, which is the intended pybind11 pattern.
  • Ensure Signal component bases are registered before Signal components in Python bindings (dpsim/src/pybind/main.cpp): The file only reorders binding setup; it does not add any component implementation, and the claimed MNA dependency hooks are not something this binding file should declare.
  • Missing explicit dependency on base signal classes in Python bindings (dpsim/src/pybind/main.cpp): The file explicitly registers the signal base classes via addSignalComponentBases(mSignal) before addSignalComponents(mSignal).
  • Ensure Simulation class is fully defined before RealTimeSimulation inherits from it (dpsim/src/pybind/main.cpp): RealTimeSimulation is bound after Simulation in the same file, and the Simulation binding is already created at line 269 before the derived class declaration.
  • SystemTopology class binding must follow all method definitions that reference it (dpsim/src/pybind/main.cpp): SystemTopology is declared as a py::class_ handle at line 267 before any Simulation methods use it, and its method bindings are intentionally attached later.
  • Use consistent naming for py::class_ handles to avoid confusion (dpsim/src/pybind/main.cpp): The file consistently uses both named handles and direct temporary bindings; the absence of a handle for some classes is a style choice, not a defect.
  • Missing override specifiers on virtual methods in SignalComponents.cpp (dpsim/src/pybind/SignalComponents.cpp): This is a pybind registration function, not a C++ virtual override site; no override specifier is applicable here.
  • Multiple inheritance in pybind bindings without virtual inheritance (dpsim/src/pybind/SignalComponents.cpp): The binding already uses py::multiple_inheritance() exactly as written on the TurbineGovernorType1 class registration.
  • Inconsistent default argument style in Simulation constructor binding (dpsim/src/pybind/main.cpp): logLevelArg(CPS::Logger::Level::off) is present in the Simulation constructor binding and is the intended replacement shown in the diff.
  • Inconsistent default argument style in RealTimeSimulation constructor binding (dpsim/src/pybind/main.cpp): logLevelArg(CPS::Logger::Level::info) is present in the RealTimeSimulation constructor binding and is the intended replacement shown in the diff.
  • Inconsistent default argument style in CIMReader constructor binding (dpsim/src/pybind/main.cpp): The CIMReader binding explicitly uses logLevelArg for both Logger::Level parameters on lines 559-560.
  • rename function to match its purpose (dpsim/src/pybind/SignalComponents.cpp): The function name addSignalComponentBases is present in the source and matches its purpose of registering multiple base classes.
  • inconsistent keyword argument naming (dpsim/src/pybind/main.cpp): The constructor argument is named with "name"_a, and the log level uses the project’s helper-style default argument form.
  • inconsistent keyword argument naming (dpsim/src/pybind/main.cpp): The constructor argument is named with "name"_a, and the log level uses the project’s helper-style default argument form.
  • inconsistent keyword argument naming (dpsim/src/pybind/main.cpp): Both CIMReader level arguments are given through logLevelArg, with the second explicitly named "comploglevel".

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

Re-checked at 2541249: nothing new.

@sonarqubecloud

sonarqubecloud Bot commented Aug 7, 2026

Copy link
Copy Markdown

…e them

Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.33333% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (fix/pybind-enum-arg-repr@f6eda27). Learn more about missing BASE report.

Files with missing lines Patch % Lines
dpsim/src/pybind/SignalComponents.cpp 95.23% 1 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                     @@
##             fix/pybind-enum-arg-repr     #654   +/-   ##
===========================================================
  Coverage                            ?   64.91%           
===========================================================
  Files                               ?      533           
  Lines                               ?    37509           
  Branches                            ?    19937           
===========================================================
  Hits                                ?    24350           
  Misses                              ?    13158           
  Partials                            ?        1           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sonarqubecloud

Copy link
Copy Markdown

This branch was successfully deployed

1 active deployment
internal — 25412491 Deployed Sep 21, 2026 by leonardocarreras via Resolve revision and images #315
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.

1 participant