Migrate transformers cli to Typer#41487
Conversation
|
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. |
Good to know! Changes in this PR looks nice. I tried again to run a simple
Yay! Will work on that since I also got informal approval from @LysandreJik
Will check that again but might be for a future PR. |
|
@gante @LysandreJik should now be ready for review. The revamped |
|
Note: I'm not 100% sure that the slow tests are running. @ydshieh would it be possible to trigger them please? (or anyone else with the permissions?) |
|
Reviewing it in batches:
|
|
While I agree we can remove the support of I get the following error: which isn't super clear: I'd tell the user that this path isn't supported anymore and to please launch a |
|
For transformers serve x chat to work well we likely need to merge the following in this PR: #41446 It throws an error currently that is fixed by the above ^ I'll merge it into main shortly |
d299b27 to
0756797
Compare
tests + fixup
0756797 to
cee44ab
Compare
@LysandreJik I addressed this comment in 05d9515. It's not so straightforward to do it but "it works". I didn't want to just make it optional and then raise an error if not passed as it would have modified the Error mesage is now: |
|
Awesome, thanks @Wauplin ! |
* Add typer-slim as explicit dependency * Migrate CLI to Typer * code quality * bump release candidate * adapt test_cli.py * Remove ./commands + adapt tests * fix quality * consistency * doctested * do not serve model in chat * style * will it fix them? * fix test * capitalize classes * Rebase * Rebase * tests + fixup tests + fixup * csutom error message * fix ? * should be good * fix caplog globally * inner caplog * last attempt * Retry * Let's try with capsys disabled --------- Co-authored-by: Lysandre <hi@lysand.re>
* Add typer-slim as explicit dependency * Migrate CLI to Typer * code quality * bump release candidate * adapt test_cli.py * Remove ./commands + adapt tests * fix quality * consistency * doctested * do not serve model in chat * style * will it fix them? * fix test * capitalize classes * Rebase * Rebase * tests + fixup tests + fixup * csutom error message * fix ? * should be good * fix caplog globally * inner caplog * last attempt * Retry * Let's try with capsys disabled --------- Co-authored-by: Lysandre <hi@lysand.re>
This PR migrates the
transformersCLI to Typer.Typer is a package build on top of click by the creator of FastAPI. It is now a dependency of
huggingface_hub, meaning it's also a dependency oftransformers. Typer simplifies arg definition and ensures consistency using type annotations, which should help with maintenance. The benefit for users is the built-in autocompletion feature that let someone dotransformers chat [TAB][TAB]to check what are the options. The--helpsection is also improved.By migrating to Typer, a longer term goal is to delegate to
huggingface_hubsome aspects of the installation and auto-update of the CLI. This will come in a second time and doesn't have to be correlated with the v5 release.CLI
--helpSide notes
Noted down some stuff while working on it. Can be addressed in later PRs.
Any command, even a simple
transformers envis currently very long due both in previous and new CLI. This is due totorchimport, no matter if it's used or not. I do think this is not good UX, especially if we want to have something liketransformers chatas entrypoint for any openai-compatible server.This is not really related to CLI only, but to lazy imports in general. I broke it down to
is_torch_availableactually importing the package, not just checking it's existence.The current
transformers serve+transformers chattwin commands are really nice. One to start a server, the other one launch a chat interface. However, I feel that the current UX forchatis too bloated since it covers both the case where a server is already started AND started a new server from a model id (or path). I do think thattransformers chatshould only be to consume an existing API. It would make the whole implementation much cleaner and the interface less bloated for the end user (currently having 4-5 arguments only to provide model name, path, address, port, and host, instead of a single "url" argument).Since this would be a breaking change, I think addressing it for v5 is the perfect timing.
=> EDIT: this is now done in this PR.
transformers chatdoes not serve a model now (making it much simpler)transformers servefeature is currently only available as a CLI. I do believe it would be best to move it to its own module so that someone could call it programmatically (e.g.from transformers import servein a notebook).TODO
typer_factoryfrom private internal (requires an update inhuggingface_hubfirst)./commandsfolder and removetransformers-legacyCLI (that I currently use for testing)transformers chatUI-only (not serving)chatandserve? + expose them as modules (e.g.from transformers import chat, serve)