-
Notifications
You must be signed in to change notification settings - Fork 4
test: pybind embed test framework (#199) #200
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
Open
alexallmont
wants to merge
18
commits into
main
Choose a base branch
from
199-pybind11-embed-test
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
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.
Open
Conversation
This file contains hidden or 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
- Initial unit test is a placeholder to demonstrate pybind11::embed functionality, constructing a FreeTensor from a context in Python API and then checking the returned pointer in C++ API. - Outstanding FIXME in CMakeLists to review linkage; running more than import results in a `double free or corruption (out)` which I think is due to using external rougpy import rather than private embedding.
- Experimental change to try running tests from PYBIND11_EMBEDDED_MODULE rather than `import roughpy`, i.e. not depend on finding an external .so file. - This adds a new static library RoughPy_StaticLib which contains the bulk of what was in RoughPy_PyModule, but with PUBLIC rather than PRIVATE linkage. This currently raises compiler warnings which will need fixing if this approach is taken. - The #defed out code at the end of `test_python_embed.cpp` still causes the same double-free issue as before, I assume because embedded pybind does none of the module caching that the interpreter does. Fix in progres.
- This reverts previous RoughPy_StaticLib change as the warnings caused by previous commit highlight legitimate linker issues. - Instead a new cmake macro roughpy_module_sources adds all roughpy files to both RoughPy_PyModule and test_RoughPy_PyModule if tests enabled, then the latter project is completely isolated. - The FIXME in tests is regarding poor reuse of one interpreter for all tests, wiping down globals between each. This avoids the `double free or corruption (out)` error but needs a neater solution.
- Trying to resolve `ModuleNotFoundError: No module named 'numpy'` on CI. Not sure if this is it but it's one of the few Win32 custom linkage fixes.
This reverts commit f936b98.
- This is in attempt to fix failing embedded python test for Win32 CI, as it's also set for pytest.
- Experimental change to try to fix import numpy error on Windows build; perhaps can be resolved by telling ctest exactly the PYTHONPATH at time of compilation
- The previous code demonstrated embedding the test in pybind statements. This instead now uses a python script and stores any test variables in globals so they can be retrieved in C++ later. - Also the import of _roughpy_embed is left to each TEST_F if required.
- Rather than a singleton with a created-once check, use a global that is initialised in the single call to `SetUpTestSuite`
- Added a custom exception to scalars.h that stores the source and destination conversion types. These can then be reported along with captured leaf information in the roughpy conversion manager. - Note point for review, these currently use `throw` rather than RPY_THROW partly because the latter macro doesn't seem to accept multiple args for new exception, and because I'm not sure if public python API exceptions should report C++ line numbers as in the macro.
- Added explicit checks for strings in conversion manager. The previous code was regarding strings as sequences which resulted in it recursing one level in and reporting "maximum nested depth reached in this context" which didn't clearly identify the bad type as cause. - Includes unit test to confirm exceptions.
- Tidying for shorter name in line with existing tests.
- Add new CMake option ROUGHPY_BUILD_TEST_PYTHON_EMBED - Option is enabled for dev preset. - Option is enabled on CI unless Windows platform to avoid getting bogged down in paths issue as this test is more for local dev currently. - Renamed `test_python_api` to `test_python_embed` for clearer meaning.
- The issue of C++ appearing in Python exceptions has been raised in a separate ticket #202
- The logic here became muddled in the switch to ROUGHPY_BUILD_TEST_PYTHON_EMBED when the original ROUGHPY_BUILD_TESTS condition was replaced, conflating the add_custom_command logic for both cases. This was causing a Windows CI `ModuleNotFoundError: No module named 'roughpy._roughpy'` error becasue the embed flag is disabled for Windows, in turn making it bypass the _roughpy copy operation. - The original code has been restored and the embed test separately underneath, duplicating the original copy _roughpy logic for each platform.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
double free or corruption (out)
which I think is due to using external rougpy import rather than private embedding.