-
Notifications
You must be signed in to change notification settings - Fork 113
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
Bring Nanobind. Fix object leak in from_numpy
#2545
Draft
ferdonline
wants to merge
74
commits into
master
Choose a base branch
from
feature/nanobind
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
74 commits
Select commit
Hold shift + click to select a range
c243a00
Bring nanobind. Using it for Vector.from_numpy
ferdonline 1cf9518
format fixes. Dont run nanobind cmake
ferdonline 53390db
Fix. clone depth 1 by default
ferdonline 0ced67c
Fix compilation w nanobind when NOT NRN_ENABLE_PYTHON_DYNAMIC
ferdonline cdf47c5
Improve nrnpy_vec_from_python so it uses list getitem when possible. …
ferdonline 9e2e0e7
Create nanobind target with better flags. No default visibility
ferdonline 05ed787
Unique nanobind targets, one for each dyn python
ferdonline 6e4d02c
Improve build as per the original script
ferdonline 79f4ac8
Merge branch 'master' into feature/nanobind
ferdonline a183cda
For an object lib use setting similar to static lib
ferdonline 45473c4
Address Sonar checks
ferdonline 166f460
Merge branch 'master' into feature/nanobind
ferdonline d166106
Avoid warnings from 3rd party includes
ferdonline a4f9219
Upgrade to v2.0.0
alkino 547baab
Merge remote-tracking branch 'origin/master'
alkino 5789e33
foobar
alkino 7ef1aa9
foobar
alkino 33384b0
Fix formatting
github-actions[bot] 6d28378
SHARED => STATIC
alkino 356756d
Merge remote-tracking branch 'origin/feature/nanobind'
alkino a7e14ac
debug
alkino d8353bd
Fix shared / object / static
alkino fb8caa8
Revert "debug"
alkino 087edbc
More
alkino 47c3167
Debug windows...
alkino b847e73
Export nrnpy_hoc
6f26ef0
Export more symbols for microsoft
b8f38bf
include
6fe9eaa
Simplify CI
2a4b45f
Revert "Simplify CI"
0e889f2
Simplify CI
60ae462
better linkage
741295f
Position is important
c280d6f
More export for windows
232dba3
Fix formatting
github-actions[bot] 4c3abd8
NB_EXPORT for RXD
1c6c4f2
Merge remote-tracking branch 'origin/feature/nanobind'
501bb93
one left
41ee749
Extern in the right place NB_EXPORT the world
alkino 59b124c
Fix formatting
github-actions[bot] 26c15ad
Simplify export
alkino 8a38487
Merge remote-tracking branch 'origin/feature/nanobind'
alkino 6d0f998
Fix formatting
github-actions[bot] 8087f23
make_time_ptr
alkino 2162bb9
Merge remote-tracking branch 'origin/feature/nanobind'
alkino c912b00
Revert "Simplify CI"
alkino d8eee43
Revert "Debug windows..."
alkino 8201d10
Miss one endif
alkino 8b7200a
shortened nb_defs.h
alkino 9832d25
Fix formatting
github-actions[bot] 430b64d
Use nanobind nb_defs
alkino 86b8964
Merge remote-tracking branch 'origin/feature/nanobind'
alkino 2729efc
Fix comments from Luc
alkino 3f537fa
Don't modify External
alkino d018120
Reduce size of modification
alkino b44c242
Fix formatting
github-actions[bot] df0d684
More verbose cmake
alkino 5cc8780
Merge remote-tracking branch 'origin/feature/nanobind'
alkino 0ffb545
Fix formatting
github-actions[bot] 016fcdc
Need recursive for nanobind
alkino b1c80b8
export all
alkino dd0bfb0
Huge clean
alkino 58f43ce
Fix cmake
alkino e936b96
failure
alkino db63912
Try
alkino 4147249
We are compiling with MINGW
alkino b87a990
Resurce only nanobind
alkino 3177806
Revert "Resurce only nanobind"
alkino 663e62f
Merge remote-tracking branch 'origin/master'
alkino dbfab15
Try with a def file
alkino c2d314a
Fix formatting
github-actions[bot] d7aa1ac
forgot nrnpy_hoc
alkino 41c50ad
Merge remote-tracking branch 'origin/feature/nanobind'
alkino 500f98c
Fix format?
alkino File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# ============================================================================= | ||
# minimal nanobind interface | ||
# ============================================================================= | ||
|
||
set(NB_DIR ${PROJECT_SOURCE_DIR}/external/nanobind) | ||
|
||
function(make_nanobind_target TARGET_NAME PYINC) | ||
add_library(${TARGET_NAME} STATIC ${NB_DIR}/src/nb_combined.cpp) | ||
target_include_directories(${TARGET_NAME} SYSTEM PUBLIC ${NB_DIR}/include) | ||
target_include_directories(${TARGET_NAME} SYSTEM PRIVATE ${NB_DIR}/ext/robin_map/include ${PYINC}) | ||
if(MSVC) | ||
# Do not complain about vsnprintf | ||
target_compile_definitions(${TARGET_NAME} PRIVATE -D_CRT_SECURE_NO_WARNINGS) | ||
else() | ||
# Generally needed to handle type punning in Python code | ||
target_compile_options(${TARGET_NAME} PRIVATE -fno-strict-aliasing) | ||
endif() | ||
target_compile_features(${TARGET_NAME} PUBLIC cxx_std_17) | ||
set_property(TARGET ${TARGET_NAME} PROPERTY POSITION_INDEPENDENT_CODE True) | ||
set_property(TARGET ${TARGET_NAME} PROPERTY INTERPROCEDURAL_OPTIMIZATION_RELEASE ON) | ||
endfunction() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
make_time_ptr | ||
ECS_insert | ||
ICS_insert | ||
set_diffusion | ||
set_tortuosity | ||
set_volume_fraction | ||
ics_set_grid_concentrations | ||
ics_set_grid_currents | ||
set_grid_concentrations | ||
set_grid_currents | ||
delete_by_id | ||
ECS_insert | ||
ICS_insert | ||
ICS_insert_inhom | ||
set_diffusion | ||
PyInit_hoc | ||
modl_reg | ||
nrn_hocobj_ptr | ||
nrnpy_set_vec_as_numpy | ||
nrnpy_set_toplevel_callbacks | ||
nrnpy_set_gui_callback | ||
get_plotshape_data | ||
nrnpy_vec_math_register | ||
nrnpy_rvp_pyobj_callback_register | ||
nrnpython_reg_real | ||
nrn_hocobj_ptr | ||
rxd_set_no_diffusion | ||
free_curr_ptrs | ||
free_conc_ptrs | ||
rxd_setup_curr_ptrs | ||
rxd_setup_conc_ptrs | ||
rxd_include_node_flux3D | ||
rxd_include_node_flux1D | ||
rxd_set_euler_matrix | ||
set_setup | ||
set_initialize | ||
set_setup_matrices | ||
set_setup_units | ||
setup_currents | ||
rxd_nonvint_block | ||
register_rate | ||
clear_rates | ||
species_atolscale | ||
remove_species_atolscale | ||
setup_solver | ||
set_num_threads | ||
get_num_threads | ||
scatter_concentrations | ||
ics_register_reaction | ||
ecs_register_reaction | ||
scatter_concentrations | ||
set_hybrid_data | ||
llgramarea | ||
llpipedfromoriginvolume | ||
find_triangles | ||
factorial | ||
degrees | ||
radians | ||
log1p | ||
vtrap | ||
nrnpy_hoc |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
--recursive
still? Just for nanobind? Can we try to do that instead in.gitmodules
? Otherwise we pull the big string and slurp in all of nmodl's submodules ⇒ lots of sources!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly this is not working as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
fetchRecurseSumodules
is only forfetch
andpull
.git submodule update
is doing acheckout
.So I will clone the
git
repository and read the code to see if there is an hidden variable to set that.Stay in touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in
git
there is 2 different things, core module written inC
, and helpers previously written inperl
but today rewrite inC
but not really integrated to the core.git submodule
command is one of those previous helper and so the integration with the core is not perfect. It does not take care ofoptions
for example.Should I tried to patch git to have a better consideration of options?
For now, I will continue to
update --init --recursive
but still no--depth=1
.Don't be afraid, I will not merge like that, just trying to continue the debugging of windows/symbol/cmake things with tmate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK :)
Thank you for looking in depth there!