pybind updates for latest master#1090
Conversation
📝 WalkthroughWalkthroughReplace std::shared_ptr holder policies with py::smart_holder in multiple pybind11 class bindings, adjust Span and PyWrapper bindings, move PatchDataVectorDouble registrations earlier in module init, and add a requires clause to PatchData’s templated constructor to restrict constructibility. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/python3/cpp_simulator.hpp (1)
35-41: Add pybind11 smart_holder include and type-casters for shared_ptr interop
- Add
#include <pybind11/smart_holder.h>insrc/python3/cpp_simulator.hpp(or a common binding header).- Define
PYBIND11_SMART_HOLDER_TYPE_CASTERSfor your shared_ptr-backed types (e.g.Sim,PHARE::amr::Hierarchy, and any other smart_holder types) alongside your type aliases.
🧹 Nitpick comments (1)
src/python3/cpp_etc.cpp (1)
44-46: Switch to smart_holder: ensure header and casters are available.Verify that pybind11/smart_holder.h is included (likely via python3/pybind_def.hpp). If not, include it explicitly here or in a common header.
If Span/PyArrayWrapper (or any related APIs) pass/return std::shared_ptr across the Python boundary, add the required smart‑holder type‑casters for those types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
res/cmake/dep/pybind.cmake(1 hunks)src/python3/cpp_etc.cpp(1 hunks)src/python3/cpp_simulator.cpp(1 hunks)src/python3/cpp_simulator.hpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit configuration file
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/python3/cpp_simulator.hpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
🧬 Code graph analysis (1)
src/python3/cpp_simulator.cpp (1)
src/python3/cpp_simulator.hpp (8)
declarePatchData(32-42)declarePatchData(32-32)declare_essential(182-194)declare_essential(182-182)declareDim(45-60)declareDim(45-45)declare_all(168-179)declare_all(168-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (1)
src/python3/cpp_simulator.cpp (1)
15-17: OK to register PatchDataVectorDouble_ earlier.*Reordering looks fine. Please confirm the later duplicate registrations were removed to avoid multiple-definition/overwrite at import.
PhilipDeegan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/python3/patch_data.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit configuration file
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/python3/patch_data.hpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
|
this is unnecessary strictly since pybind was fixed, but maybe we want it regardless |
pin pybind version and use smart_holder, block non-constructible arguments due to catch-all Args...
2 participants
seeing compile errors since pybind/pybind11#5866
should be resolved with the additional
requiresalso updated to smart_holders instead of shared_ptrs as noted here https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html#py-smart-holder