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

pyo3: update to 0.17 #1066

Merged
merged 3 commits into from
Oct 5, 2022
Merged

pyo3: update to 0.17 #1066

merged 3 commits into from
Oct 5, 2022

Conversation

davidhewitt
Copy link
Contributor

Still a WIP; doing this I found an unintended API change in PyO3 which I think I can push a patch release for.

@@ -2,7 +2,7 @@
name = "tokenizers-python"
version = "0.11.0"
authors = ["Anthony MOI <m.anthony.moi@gmail.com>"]
edition = "2018"
edition = "2021"
Copy link
Contributor Author

@davidhewitt davidhewitt Sep 20, 2022

Choose a reason for hiding this comment

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

@Narsil if you cherry-pick 8691345 onto your branch for #1064 I think it may fix the issue.

The thinking is that edition 2021 uses Cargo's newer resolver = "2" which won't add the auto-initialize feature from the pyo3 dev-dependency during the manylinux build, which I think will avoid the problem.

@Narsil
Copy link
Collaborator

Narsil commented Sep 21, 2022

Hey,

Thanks to your work I was able to make it work (by removing the dev-dependencies with auto-initialize). I added the dependency as a pure feature, which seems to not work properly when doing cargo test --features test but this is not the most important to fix soon (there's a bunch of failures to link to python functions, even if I am in a shared Python 3.10.7 environement).

Thanks a ton for this work, and I'm all up for upgrading to 0.17 !

@Narsil
Copy link
Collaborator

Narsil commented Sep 22, 2022

@davidhewitt davidhewitt marked this pull request as ready for review October 4, 2022 07:37
@davidhewitt
Copy link
Contributor Author

@Narsil I've since released PyO3 0.17.2 and this branch now builds for me locally, should be good for review and also it would be great to have the CI run approved to confirm it passes your CI.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 5, 2022

The documentation is not available anymore as the PR was closed or merged.

@Narsil
Copy link
Collaborator

Narsil commented Oct 5, 2022

Hey @davidhewitt Thanks for the work.

I took the liberty of rebasing (to prevent merge issues) and modifying the makefile line that was failing !
If this works this will get merged !

@Narsil Narsil merged commit 8129dd3 into huggingface:main Oct 5, 2022
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.

3 participants