-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Replace upstream.yml with new python312.yml #4397
Conversation
Hi @vstinner, just hoping that a minute of your time may give us a clue: Does the traceback below ring any bells? — It involves Stack frame
The same pybind11 code works with Python 3.6 through 3.11 and 3.12 alpha.1 (freshly confirmed via interactive testing). What change in 3.12, between alpha.1 and alpha.2 (also freshly confirmed), could have gotten us here? For easy reference, here is:
|
I don't like the name change. Why change the name once a year when you can just have a simpler name that doesn't change every year? Also, we've used upstream for other upstream checks, like dev versions of PyPy. I'll probably implement something like pypa/build#543 soon, and might even drop upstream.yml entirely. |
Because then it's very obvious what it does. "upstream" is very opaque, it took me a while to realize (back then).
That's very confusing. Meaningful naming has a lot of value.
Sounds good, but could we please have this change until then? It's important for my work right now. I could rename it back, if it gets replaced soon anyway it doesn't matter one way or another. |
If you do rename a file, it needs to be a separate commit then changes, so git can track the difference. As it is, it's just a brand new file. I would recommend renaming it back, we can review and merge based on this, then if you really want the name changed, maybe bring that up separately. Though I'm against the name change. |
Done. I'm OK to keep the name if that's preferred. Just adding my thoughts for completeness: There are several things that are highly specific to Python 3.12 in its current state, and are likely to change as 3.12 evolves. By the time 3.13 comes around, likely there are a bunch of other things here and there they I learned or changed in the environment. Usually I start over ~fresh in such a situation, bringing in everything I know at that time. Doing that under a new name makes it easier to follow for others; it's immediately obvious that there was some sort of discontinuity, and why. |
|
||
env: | ||
PIP_ONLY_BINARY: numpy | ||
# For cmake: | ||
VERBOSE: 1 |
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 remove VERBOSE just to add it to every line below?
# python -m pip install --prefer-binary numpy # SLOW | ||
# python -m pip install --prefer-binary scipy # FAILED ~Nov 2022 |
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.
These generally will only work after RC1 is out, and by that time, we should be including the next release in our regular, required CI. I'd say just keep them out of this.
if: runner.os == 'Linux' | ||
run: python -m pip install pytest-github-actions-annotate-failures | ||
- name: Show CMake version | ||
run: cmake --version |
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.
This should get printed in our regular build? If not, we could add it (only if pybind11 is the master project).
# python-3.12.0-alpha.1-linux-22.04-x64.tar.gz PASSED | ||
# python-3.12.0-alpha.2-linux-22.04-x64.tar.gz FAILED | ||
# python-3.12.0-alpha.3-linux-22.04-x64.tar.gz FAILED | ||
# free(): invalid pointer | ||
# Custom PyConfig | ||
# /home/runner/work/pybind11/pybind11/tests/test_embed/test_interpreter.cpp:175 | ||
# /home/runner/work/pybind11/pybind11/tests/test_embed/test_interpreter.cpp:179: FAILED: | ||
# gdb traceback points here: | ||
# https://github.com/python/cpython/blame/54289f85b2af1ecf046089ddf535dda1bdf6af24/Python/import.c#L524 | ||
# - name: C++ tests C++11 | ||
# run: cmake --build build11 --target cpptest -j 2 |
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.
This is the only 3.12 specific part. I'd rather record logs of things happening outside the repo, certainly not in the CI files. I'd just make an issue and dump this in there. One issue per Python release would make much more sense than renaming a CI file every year. It's easier to update an issue than commit a change to the files.
- name: Setuptools helpers test | ||
run: pytest tests/extra_setuptools | ||
|
||
- name: Clean directory | ||
run: git clean -fdx |
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 spend time deleting things if this is the last step?
@@ -7,108 +6,137 @@ on: | |||
|
|||
concurrency: | |||
group: upstream-${{ github.ref }} | |||
cancel-in-progress: true | |||
cancel-in-progress: false |
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 is this false? If you push a new commit, you probably don't care about the old one finishing, regardless of the fact this is a development build?
# - name: C++ tests C++17 | ||
# run: cmake --build build17 --target cpptest |
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.
I'm not sure what the benefit of not running the tests to make the not-required and not-default tests pass is.
This PR was transferred to google/pybind11clif#30006. Note: At the moment, the http://github.com/google/pywrapcc repo is effectively a branch of smart_holder, to support the PyCLIF-pybind11 integration work. Currently pywrapcc is not recommended for other uses. |
* Transfer PR pybind#4329 from master to smart_holder branch, STEP 1. The patch .rej below are resolved, but THIS STATE DOES NOT BUILD: ``` clang++ -o pybind11/tests/test_constants_and_functions.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:11: In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/pybind11_tests.h:3: In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/eval.h:14: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1781:9: error: static_assert failed due to requirement '!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype' "py::class_ holder vs type_caster mismatch: missing PYBIND11_TYPE_CASTER_BASE_HOLDER(T, ...) or collision with custom py::detail::type_caster<T>?" static_assert(!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2460:11: note: in instantiation of function template specialization 'pybind11::class_<MyEnum>::class_<>' requested here : class_<Type>(scope, name, extra...), m_base(*this, scope) { ^ /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:106:5: note: in instantiation of function template specialization 'pybind11::enum_<MyEnum>::enum_<>' requested here py::enum_<MyEnum>(m, "MyEnum") ^ 1 error generated. ``` ________ ``` rwgk.c.googlers.com:~/forked/pybind11 $ patch -p 1 < ~/native_enum_git_diff_master_2022-11-19+131942.patch patching file .github/workflows/python312.yml patching file CMakeLists.txt Hunk #1 FAILED at 111. Hunk #2 succeeded at 138 (offset 5 lines). 1 out of 2 hunks FAILED -- saving rejects to file CMakeLists.txt.rej patching file include/pybind11/cast.h Hunk #1 FAILED at 12. Hunk #2 succeeded at 78 (offset 30 lines). Hunk #3 succeeded at 1173 (offset 41 lines). 1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/cast.h.rej patching file include/pybind11/detail/abi_platform_id.h patching file include/pybind11/detail/cross_extension_shared_state.h patching file include/pybind11/detail/internals.h Hunk #1 FAILED at 16. Hunk #2 succeeded at 109 (offset 1 line). Hunk #3 FAILED at 203. Hunk #4 succeeded at 398 (offset 11 lines). Hunk #5 succeeded at 457 (offset 11 lines). 2 out of 5 hunks FAILED -- saving rejects to file include/pybind11/detail/internals.h.rej patching file include/pybind11/detail/native_enum_data.h patching file include/pybind11/detail/type_map.h patching file include/pybind11/embed.h patching file include/pybind11/native_enum.h patching file include/pybind11/pybind11.h Hunk #1 FAILED at 12. Hunk #2 succeeded at 1269 (offset 1 line). Hunk #3 succeeded at 2457 (offset 255 lines). 1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/pybind11.h.rej patching file include/pybind11/pytypes.h patching file tests/CMakeLists.txt Hunk #1 succeeded at 160 (offset 18 lines). patching file tests/conftest.py patching file tests/extra_python_package/test_files.py Hunk #2 FAILED at 47. 1 out of 2 hunks FAILED -- saving rejects to file tests/extra_python_package/test_files.py.rej patching file tests/test_embed/test_interpreter.cpp patching file tests/test_enum.cpp patching file tests/test_enum.py patching file tests/test_native_enum.cpp patching file tests/test_native_enum.py ``` * Make `smart_holder` code compatible with `type_caster_enum_type` * Fix `if` condition guarding `Unable to cast native enum type to reference` * WIP native_enum_add_to_parent * PYBIND11_SILENCE_MSVC_C4127 * Transfer upstream.yml/python312.yml changes from PR pybind#4397 * clang-tidy (clang 15) auto-fix * Remove python312.yml: this is handled separately under PR #30006 * Fixes for ruff * Replace `PyEval_GetBuiltins()` in `finalize_interpreter()` with `get_python_state_dict()` * Bug fix: Ensure `state_dict` is destroyed before `Py_Finalize()` * Restore tests/test_embed/test_interpreter.cpp from google_pywrapcc_main * Restore include/pybind11/embed.h from google_pywrapcc_main * Back out all native_enum changes, leaving only the cross_extension_shared_state changes. * Undo unrelated one-line change (from `auto` to `internals`).
…. (#30005) * Transfer PR pybind#4329 from master to smart_holder branch, STEP 1. The patch .rej below are resolved, but THIS STATE DOES NOT BUILD: ``` clang++ -o pybind11/tests/test_constants_and_functions.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:11: In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/pybind11_tests.h:3: In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/eval.h:14: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1781:9: error: static_assert failed due to requirement '!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype' "py::class_ holder vs type_caster mismatch: missing PYBIND11_TYPE_CASTER_BASE_HOLDER(T, ...) or collision with custom py::detail::type_caster<T>?" static_assert(!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2460:11: note: in instantiation of function template specialization 'pybind11::class_<MyEnum>::class_<>' requested here : class_<Type>(scope, name, extra...), m_base(*this, scope) { ^ /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:106:5: note: in instantiation of function template specialization 'pybind11::enum_<MyEnum>::enum_<>' requested here py::enum_<MyEnum>(m, "MyEnum") ^ 1 error generated. ``` ________ ``` rwgk.c.googlers.com:~/forked/pybind11 $ patch -p 1 < ~/native_enum_git_diff_master_2022-11-19+131942.patch patching file .github/workflows/python312.yml patching file CMakeLists.txt Hunk #1 FAILED at 111. Hunk #2 succeeded at 138 (offset 5 lines). 1 out of 2 hunks FAILED -- saving rejects to file CMakeLists.txt.rej patching file include/pybind11/cast.h Hunk #1 FAILED at 12. Hunk #2 succeeded at 78 (offset 30 lines). Hunk #3 succeeded at 1173 (offset 41 lines). 1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/cast.h.rej patching file include/pybind11/detail/abi_platform_id.h patching file include/pybind11/detail/cross_extension_shared_state.h patching file include/pybind11/detail/internals.h Hunk #1 FAILED at 16. Hunk #2 succeeded at 109 (offset 1 line). Hunk #3 FAILED at 203. Hunk #4 succeeded at 398 (offset 11 lines). Hunk #5 succeeded at 457 (offset 11 lines). 2 out of 5 hunks FAILED -- saving rejects to file include/pybind11/detail/internals.h.rej patching file include/pybind11/detail/native_enum_data.h patching file include/pybind11/detail/type_map.h patching file include/pybind11/embed.h patching file include/pybind11/native_enum.h patching file include/pybind11/pybind11.h Hunk #1 FAILED at 12. Hunk #2 succeeded at 1269 (offset 1 line). Hunk #3 succeeded at 2457 (offset 255 lines). 1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/pybind11.h.rej patching file include/pybind11/pytypes.h patching file tests/CMakeLists.txt Hunk #1 succeeded at 160 (offset 18 lines). patching file tests/conftest.py patching file tests/extra_python_package/test_files.py Hunk #2 FAILED at 47. 1 out of 2 hunks FAILED -- saving rejects to file tests/extra_python_package/test_files.py.rej patching file tests/test_embed/test_interpreter.cpp patching file tests/test_enum.cpp patching file tests/test_enum.py patching file tests/test_native_enum.cpp patching file tests/test_native_enum.py ``` * Make `smart_holder` code compatible with `type_caster_enum_type` * Fix `if` condition guarding `Unable to cast native enum type to reference` * WIP native_enum_add_to_parent * PYBIND11_SILENCE_MSVC_C4127 * Transfer upstream.yml/python312.yml changes from PR pybind#4397 * clang-tidy (clang 15) auto-fix * Remove python312.yml: this is handled separately under PR #30006 * Fixes for ruff * Replace `PyEval_GetBuiltins()` in `finalize_interpreter()` with `get_python_state_dict()` * Bug fix: Ensure `state_dict` is destroyed before `Py_Finalize()` * Restore tests/test_embed/test_interpreter.cpp from google_pywrapcc_main * Restore include/pybind11/embed.h from google_pywrapcc_main * Undo unrelated one-line change (from `auto` to `internals`). * Add missing `test_class_with_enum` * Add note to docs/classes.rst pointing to PR #30005
Description
Add new python312.yml, replacing obsolete upstream.yml. The new name is to make it completely obvious what the workflow is for. (We will have to update the name only once a year.)
Main purpose: testing builtins / state dict change started under PR #4293 and continued under the pair of PRs #4329/#4349 (master/smart_holder).
The Python312 workflow was working completely with python-3.12.0-alpha.1-linux-22.04-x64.tar.gz, but the C++ tests fail with alpha.2 and alpha.3. Debugging led deep into the Python sources. This is captured in a comment; further debugging is left for later (help appreciated). The main purpose (above) is sufficiently met via the Python tests + interactive testing of test_embed (ignoring the one failures).
Suggested changelog entry: