Skip to content

Install the sentencepiece headers and fix include path #110

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

Merged
merged 1 commit into from
Jul 21, 2025

Conversation

swolchok
Copy link
Contributor

I'm not sure exactly why, but we weren't installing the sentencepiece headers. We also need to set our include path to point at our installed headers.

@swolchok swolchok requested a review from larryliu0820 July 21, 2025 21:40
@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 21, 2025
CMakeLists.txt Outdated
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/pytorch/tokenizers
FILES_MATCHING
PATTERN "sentencepiece_processor.h"
PATTERN "sentencepiece_trainer.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How sure are you? I see the installation code in sentence piece, but without this I did not see these headers getting installed anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, spoke in person, I didn't realize that the request was just to remove sentencepiece_trainer.h not the whole install() call.

I'm not sure exactly why, but we weren't installing the sentencepiece headers. We also need to set our include path to point at our installed headers.
@swolchok swolchok force-pushed the install-sentencepiece-headers branch from 6f81256 to 83aeb20 Compare July 21, 2025 21:57
@swolchok swolchok merged commit f09feca into main Jul 21, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants