Skip to content
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

support exec, eval #299

Merged
merged 37 commits into from
Sep 22, 2023
Merged

support exec, eval #299

merged 37 commits into from
Sep 22, 2023

Conversation

nschloe
Copy link
Contributor

@nschloe nschloe commented Sep 18, 2023

I took pybind11's eval.h plus tests and adapted it for nanobind. I had to remove eval_file as I couldn't find a stable-ABI equivalent for opening files (done with _Py_fopen_obj in pybind11).

@wjakob I believe this is ready for the review. The test failures seem unrelated.

@nschloe nschloe changed the title [DRAFT] first shot at exec/eval etc. support exec, eval Sep 19, 2023
Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

This looks like a great start! Here is some feedback on this first version.

include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
@wjakob
Copy link
Owner

wjakob commented Sep 20, 2023

The CMake thing happens sometimes, unfortunately. Not sure what that is all about -- the next time you update the PR, it will likely succeed.

@nschloe nschloe requested a review from wjakob September 20, 2023 08:36
Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

This looks like it is close to mergeable. One remaining thing that is missing is basic Sphinx documentation that could likely be repurposed from pybind11.

include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
include/nanobind/nanobind.h Outdated Show resolved Hide resolved
include/nanobind/nb_eval.h Outdated Show resolved Hide resolved
@wjakob
Copy link
Owner

wjakob commented Sep 20, 2023

One last thing I remembered: besides copying the docs from nanobind, can you add the function signatures and document then added enum? This should go to docs/api_extra.rst, which documents opt-in parts of the nanobind API.

@nschloe nschloe requested a review from wjakob September 20, 2023 13:54
@nschloe
Copy link
Contributor Author

nschloe commented Sep 20, 2023

I could actually make eval_file work as well. Let me try and have a look.

@nschloe
Copy link
Contributor Author

nschloe commented Sep 20, 2023

Let's forget about that; I'm getting entangled in segfaults here. To be added as a later step. :)

@wjakob
Copy link
Owner

wjakob commented Sep 21, 2023

Ok. Ready for another review or potentially merge?

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

Just a few minor remaining things I noticed.

include/nanobind/eval.h Outdated Show resolved Hide resolved
include/nanobind/eval.h Outdated Show resolved Hide resolved
docs/utilities.rst Outdated Show resolved Hide resolved
include/nanobind/eval.h Outdated Show resolved Hide resolved
tests/test_eval.cpp Outdated Show resolved Hide resolved
docs/api_extra.rst Outdated Show resolved Hide resolved
docs/api_extra.rst Outdated Show resolved Hide resolved
@nschloe
Copy link
Contributor Author

nschloe commented Sep 21, 2023

Ready for another review

Always ready! :) (Thanks for the good reviews.)

@nschloe nschloe requested a review from wjakob September 21, 2023 09:57
@wjakob wjakob merged commit 47a250f into wjakob:master Sep 22, 2023
21 checks passed
@wjakob
Copy link
Owner

wjakob commented Sep 22, 2023

Merged, thank you for your work!

@nschloe nschloe deleted the add-exec branch September 22, 2023 07:19
@nschloe
Copy link
Contributor Author

nschloe commented Sep 22, 2023

Thanks again for walking me through the process!

@nschloe
Copy link
Contributor Author

nschloe commented Sep 28, 2023

80pqr8.jpg

@wjakob
Copy link
Owner

wjakob commented Sep 28, 2023

You'll have to be a lot more patient ;). There is a huge issue that I want to fix in an upcoming release (#307), and which requires some stabilization until then. If you cannot wait, just reference a particular commit of this repository (supported by pip, requirements.txt, etc.)

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.

2 participants