-
Notifications
You must be signed in to change notification settings - Fork 180
Add package tiktoken #1392
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
base: master
Are you sure you want to change the base?
Add package tiktoken #1392
Conversation
Signed-off-by: zlaazlaa <2889827787@qq.com>
mhsmith
left a comment
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.
Thanks very much. In order to merge this, you'll need to:
- Deal with the comments below.
- Add a basic test script, as described in server/pypi/README.md.
- Let me know which combinations of Python version and ABI you've tested. You don't need to do them all, but you should do at least one.
| + # Chaquopy: Add abi3-py312 feature, which depends on the Python version being built for. | ||
| + # In this case, it's for Python 3.12. | ||
| + "abi3-py312", |
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.
Based on the cryptography recipe, we can set this to the minimum supported version of Python, and then build it for any later version. So can we reduce this to Python 3.10 or even lower?
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.
Based on the
cryptographyrecipe, we can set this to the minimum supported version of Python, and then build it for any later version. So can we reduce this to Python 3.10 or even lower?
I tested the following compilation situation of tiktoken:
- Under Python 3.10, both version 0.8.0 and 0.9.0 failed to compile. It was not until version 0.7.0 that they were successfully compiled.
- The 0.9.0 version was successfully compiled under Python 3.12.
Do you think we should be using the old version of tiktoken (0.7.0) and abi3-py310 in the warehouse, or should we use the latest version of tiktoken (0.9.00) and abi3-py312 instead?
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.
Let's stay on the latest version of tiktoken. For future reference, the errors when reducing the abi3 to py310 and building against Python 3.10 are apparently all to do with the buffer API. Here are the first couple:
error[E0412]: cannot find type `Py_buffer` in module `pyo3::ffi`
--> src/py.rs:185:31
|
185 | view: *mut pyo3::ffi::Py_buffer,
| ^^^^^^^^^ not found in `pyo3::ffi`
error[E0425]: cannot find value `PyBUF_WRITABLE` in module `pyo3::ffi`
--> src/py.rs:191:32
|
191 | if (flags & pyo3::ffi::PyBUF_WRITABLE) == pyo3::ffi::PyBUF_WRITABLE {
| ^^^^^^^^^^^^^^ not found in `pyo3::ffi`
However, Python 3.11 builds fine.
Signed-off-by: zlaazlaa <2889827787@qq.com>
|
test passed: |
|
The tests pass from Python 3.11 to 3.13 on all my ARM test devices and emulators, and on x86_64 and x86 with API level 24. But on x86_64 with API level 35, it hangs on the first test. This happens on Python 3.11 and 3.12, regardless of whether it's built against abi3-py311 or abi3-py312. I don't see anything relevant in the logs. What ABI and API level did you test on? It's possible this could be resolved by moving to a different version of tiktoken, but I'll leave it to you to try that. |
Referenced b6d8f3c