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

[shortfin] Add extras_require for optional deps. #240

Closed
wants to merge 2 commits into from

Conversation

ScottTodd
Copy link
Member

@ScottTodd ScottTodd commented Sep 30, 2024

Progress on #130

These optional dependencies can be installed with e.g. pip install {INDEX URL HERE} shortfin[server].

  • I'm not sure which names we want and with which packages. How many is too many? 5 seems like a lot.
  • YMMV with Python 3.13t (free-threaded). Packages that don't yet distribute binaries for 3.13t may need to be built from source, which might require rust or other local tools.

We can share these requirements with requirements-*.txt files using this technique, if we want: https://github.com/nod-ai/SHARK-Platform/blob/72665fc0e3e8d1ed3b4e7eacdfa9c493e2ba0852/sharktank/setup.py#L46-L57 https://github.com/nod-ai/SHARK-Platform/blob/72665fc0e3e8d1ed3b4e7eacdfa9c493e2ba0852/sharktank/setup.py#L100-L105

Docs: https://setuptools.pypa.io/en/latest/userguide/dependency_management.html

shortfin/setup.py Outdated Show resolved Hide resolved
@marbre
Copy link
Collaborator

marbre commented Oct 1, 2024

  • I'm not sure which names we want and with which packages. How many is too many? 5 seems like a lot.

I don't have strong feelings as well and I am not sure about what names we really want to have. But I agree that 5 seems like a lot, especially as

  • YMMV with Python 3.13t (free-threaded). Packages that don't yet distribute binaries for 3.13t may need to be built from source, which might require rust or other local tools.

We actually don't want to pull in some of the deps if installing 3.13t at all. Within my early experiments ONNX failed to install which is why I added [requirements-tests-nogil.txt](https://github.com/nod-ai/SHARK-Platform/blob/main/shortfin/requirements-tests-nogil.txt).

We can share these requirements with requirements-*.txt files using this technique, if we want:

There might be even more options if we think about moving modernizing the project (https://packaging.python.org/en/latest/guides/modernize-setup-py-project/) and moving dependencies to our pyproject.toml (see https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#dependencies-and-requirements) as long as this works in combination with a setup.py at all. But that can also be something for a follow up.

Side note: For nighties (as long as we don't publish via PyPI) we could add direct URL dependencies (see https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#direct-url-dependencies). However, as such packages cannot be published on PyPI this is nothing we want for releases.

@ScottTodd
Copy link
Member Author

ScottTodd commented Oct 2, 2024

@stellaraccident we discussed this a bit today. Seems like we'll want a meta-package instead of putting more deps in shortfin itself?

I was referencing this from #130 when I put together this draft PR:

Python deps

shortfin does not have any Python deps for minimal functionality, but it is expected that a number of deps will be useful for various applications of it, and it may provide features based on these deps in an optional fashion that people can use if helpful/available:

  • pytest (for testing)
  • numpy
  • torch
  • uvicorn (or other ASGI web server)
  • iree-turbine (for torch compilation)
    • Transitive deps on iree-compiler and iree-runtime
  • onnx (for onnx compilation)
  • etc

@ScottTodd
Copy link
Member Author

  • I'm not sure which names we want and with which packages. How many is too many? 5 seems like a lot.

I don't have strong feelings as well and I am not sure about what names we really want to have. But I agree that 5 seems like a lot

Um: https://pypi.org/project/transformers/

Provides-Extra: accelerate, agents, all, audio, benchmark, codecarbon, deepspeed, deepspeed-testing, dev, dev-tensorflow, dev-torch, flax, flax-speech, ftfy, integrations, ja, modelcreation, natten, onnx, onnxruntime, optuna, quality, ray, retrieval, ruff, sagemaker, sentencepiece, serving, sigopt, sklearn, speech, testing, tf, tf-cpu, tf-speech, tiktoken, timm, tokenizers, torch, torch-speech, torch-vision, torchhub, video, vision

https://github.com/huggingface/transformers/blob/d7950bff82b18c823193d17d72188c5e46d06c83/setup.py#L264-L421

Well there's an upper limit to keep in mind :P

@ScottTodd
Copy link
Member Author

Here's what a meta package could look like instead of (or in addition to) these extras for shortfin itself: #244

@ScottTodd
Copy link
Member Author

Closing this in favor of a meta-package approach.

@ScottTodd ScottTodd closed this Oct 18, 2024
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.

2 participants