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

Do not vendorize third-party packages #848

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vvzen
Copy link

@vvzen vvzen commented May 9, 2022

Hi all!

I'm opening this PR as a discussion starter, since this repo has disabled the 'Issues' (or at least, I can't find a way to create a new Issue). So it seems that the only way to provide feedback to this open source project is to open a PR (which, sorry, is also a bit of a weird way to handle the management of an open source project, but anyway.....).

I personally have very strong views on the practice of vendorizing dependencies, not because I'm a evil programmar - but because I had to deal with the unnecessary complexity that it introduces, especially if you're trying to properly manage dependencies in a big studio. The TL;DR is that vendorizing is a 'evil' shortcut that sacrifices clean and good dependency management for the sake of shipping software "here/now".

Just imagine if Linux didn't have rpm/yum/apk and everybody just vendorized everything all the time: how bloated would every single binary be? It's a bit like statically linking everything all the time.....

Anyway - I hope I'll get some feedback on why this project has decided to vendor its dependencies, because this has made my life as a Pipeline TD in a studio that uses sgtk way harder than I feel it needed to be. Maybe I'm missing some clear advantage here.

I'd be happy to show (technically) how one could go, imho, about removing this burden from the consumer, or at least making it optional to also download your vendored packages.

Here follow my (rewritted) commit notes:

  • Vendorizing your dependencies forces the consumers of your API to
    include these dependencies as well (or to fork your project), which is
    not good practice in case they have a more sane/programmatic/automated
    way of handling runtime dependencies (eg: conda, rez, etc.).
    Good common practice is to have a "recipe" of your dependencies (eg: a
    requirements.txt, an environment.yaml, a pyproject.toml) etc. so that
    your consumers can easily integrate these dependencies into their own
    workflow for dependency management, instead of forcing them to use
    whatever version you chose. Even if they don't end up in their
    PYTHONPATH, consumers of the API still have to deal with breaking
    changes in these dependencies. For example, I (as a vfx studio) need to
    update our tk-core to a version that has merged
    SG-16968 [tk-core] Upgrade PyYaml for Python 3 to latest version #818 in order to have
    pyyaml run fine in python3. A problem that I wouldn't have to deal with
    at all if you didn't vendorize it in the first place (have my own sane way
    of dealing with having a pyyaml for py2 and one for py3).

  • Some interesting ideas: https://gist.github.com/datagrok/8577287
    and https://dephell.readthedocs.io/index-vendor.html

  • Shameless plug on how to improve how to handle this: https://valerioviperino.me/conda/for-pipeline-tds

- Vendorizing your dependencies forces the consumers of your API to
  include these dependencies as well (or to fork your project), which is
  not good practice in case they have a more sane/programmatic/automated
  way of handling runtime dependencies (eg: conda, rez, etc.).
  Good common practice is to have a "recipe" of your dependencies (eg: a
  requirements.txt, an environment.yaml, a pyproject.toml) etc. so that
  your consumers can easily integrate these dependencies into their own
  workflow for dependency management, instead of forcing them to use
  whatever version you chose. Even if they don't end up in their
  PYTHONPATH, consumers of the API still have to deal with breaking
  changes in these dependecies. For example, I (as a vfx studio) need to
  update our tk-core to a version that has merged
  shotgunsoftware#818  in order to have
  pyaml run fine in python3. A problem that I wouldn't have to deal with
  at all f youd didn't vendorize it in the first place.
- Some interesting ideas: https://gist.github.com/datagrok/8577287
- Shameless plug: https://valerioviperino.me/conda/for-pipeline-tds
@reformstudios
Copy link
Contributor

Good points, well made. :thumbs-up:

@carlos-villavicencio-adsk
Copy link
Contributor

Thank you for sharing this idea. We're also reviewing this internally for future improvements. In the meantime feel free to share this or any other idea in our community forum.

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