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

chore: Remove CLI - this was originally intended for local development #1442

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

bryantbiggs
Copy link
Contributor

@bryantbiggs bryantbiggs commented Jan 22, 2024

Removing the CLI that was originally intended for local development

The CLI functionality has been split out into its own crate. This means the main tokenizers project doesn't have to contend with compiling the clap library nor messing with enabling/disabling the CLI feature flag. This also allows the CLI to be published to be made available for use through crates, Homebrew, etc. if that is of interest.

With splitting out the CLI, workspaces are added - this also gives the benefit of being able to run the CLI from anywhere within the project but not bring in the clap dependency into tokenizers. A lot of the crate boilerplate attributes are standardized in the root Cargo.toml and inherited by the individual crates. Currently both crates share the same version but this can easily be changed if thats desired.

I don't think this is a breaking change since my understanding is that the CLI is more of an internal developer feature, but please correct me if I am wrong on that assumption

tokenizers/Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@bryantbiggs bryantbiggs changed the title feat: Split CLI out into its own crate separate feat: Split CLI out into its own separate crate Jan 22, 2024
@Narsil
Copy link
Collaborator

Narsil commented Jan 22, 2024

Hi, thanks for this very nice PR and detailed work.

Do you have any reason why deactivating cli in your deployments was not enough ?

You are correct that the CLI is not highly maintained and not really a core dependency, although such a change would be breaking a require 0.16 to be releaed imho.

For the name, my instinct would be tokenizers-cli since it would be more in line with other HF cli we have out there (huggingface-cli, transformers-cli etc..).
Although the tendency has been more to deprecate them (to not have a bazillion CLIs and focus on a handful)
My take would actually be to just remove cli from the default features in 0.16, as it would be simpler overall implementation wise.

@ArthurZucker what's your opinion ?

@bryantbiggs
Copy link
Contributor Author

My take would actually be to just remove cli from the default features in 0.16, as it would be simpler overall implementation wise.

Even better!

Do you have any reason why deactivating cli in your deployments was not enough ?

No, nothing in particular - it was more of a bit of confusion when I was looking at the source and saw cli tucked in there behind a feature flag

@ArthurZucker
Copy link
Collaborator

Down to remove it as well 😉

@bryantbiggs bryantbiggs changed the title feat: Split CLI out into its own separate crate chore: Remove CLI - this was originally intended for local development Jan 23, 2024
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@bryantbiggs
Copy link
Contributor Author

Done!

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

LGTM thanks for cleaning

@ArthurZucker ArthurZucker merged commit 72a1973 into huggingface:main Feb 13, 2024
12 checks passed
@bryantbiggs bryantbiggs deleted the feat/split-cli branch February 13, 2024 03:27
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.

4 participants