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

Reduce/Eliminate dependencies #78

Open
cs01 opened this issue Aug 6, 2019 · 6 comments
Open

Reduce/Eliminate dependencies #78

cs01 opened this issue Aug 6, 2019 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@cs01
Copy link

cs01 commented Aug 6, 2019

We're working on some updates to python resolution when creating virtual environments from within virtual environments with nox in wntrblm/nox#231. It was suggested to use pythonfinder rather than rolling our own python resolver, which is rather tricky as you know.

We were going to discuss it even though the nox owner is hesitant to add new dependencies to nox. Then it was noted that pythonfinder has many dependencies, and the idea was rejected outright. The number of dependencies may become a deterrent to pythonfinder adoption, so I wanted to create an issue to let you know, and perhaps consider reducing/eliminating its dependencies.

@pfmoore
Copy link

pfmoore commented Aug 6, 2019

A quick review shows:

  • attrs - referenced in setup.cfg but not used
  • cached-property - referenced in setup.cfg but not used
  • crayons - only used in cli
  • click - only used in cli (secho is imported in pythonfinder but never used)
  • six - used everywhere
  • packaging - used for the LegacyVersion and Version classes, which are mostly only used in type annotations.
  • vistir - used quite a lot, but looks like only a tiny subset of the package's functionality, so pulling in everything is a lot of unneeded overhead.

Overall, it seems reasonably easy to limit the dependencies. Many of them seem like they are unused or reasonably easy to avoid. Making the CLI interface an optional extra would help (and would seem reasonable - for most people, this project will be used as a pure library, and the CLI is unneeded).
Finding alternatives for the features used from vistir would be a huge saving, as well - most of the uses seem to be compatibility versions of features in Python 3, such as pathlib and lru_cache, so maybe use core features on Python 3 and conditionally depend on vistir for Python 2?

I should also point out that I'm not against depending on other libraries, but there's a balance to be had. In this instance, it was pulling in requests and all its dependencies in particular that shocked me - that plus something that we were planning on using as a pure library pulling in a command line package like click.

@pradyunsg pradyunsg changed the title reduce/eliminate dependencies Reduce/Eliminate dependencies Aug 6, 2019
@pradyunsg
Copy link

I have much less skin in this compared to @techalchemy but I'm on board for doing this.

@pfmoore
Copy link

pfmoore commented Aug 6, 2019

I'd be willing to develop a PR for this, but (a) making the CLI an extra may impact users of the project, so I'd like confirmation that it's an acceptable change before I go ahead, and (b) I'd like to know better why vistir was chosen before I try eliminating it (mostly because I don't want to get sucked into philosophical debates about what it's OK to depend on).

@frostming frostming added the enhancement New feature or request label Aug 8, 2019
@techalchemy
Copy link
Member

Sorry for the highly delayed response here -- I have no strong opinions here as long as functionality is retained. This was developed as an ecosystem library when I refactored it out of pipenv.

vistir is just a library I maintain which provides a number of useful compatibility shim for things like encodings, useful context managers, output handling, etc. that I wind up needing in most of these libraries. Most likely I refactored some things out of here and over into there. I have no issue with moving functionality back into here to reduce dependencies. Completely agree that there are a lot here.

@techalchemy
Copy link
Member

As a side note I'll be pushing release automation for this library and am happy to add any of the people in this thread as committers / maintainers etc. Feel free to let me know and I'd be happy to add some release documentation and such.

@cs01
Copy link
Author

cs01 commented Dec 23, 2019

@gaborbernat says virtualenv and tox will soon have code that can do this, so we might want to consider using/copying that implementation once it's landed (any updates here, @gaborbernat?). wntrblm/nox#231 (comment)

techalchemy added a commit that referenced this issue Jan 7, 2020
- Remove `vistir` and `crayons`
- Reimplement heavily relied-upon functionality
- Copy over `vistir` utilities used heavily in tests to avoid this as a
  test dependency as well
- Rewrite mocks that relied on vistir-specific invocations
- Regenerate lockfile
- Begins to address #78

Signed-off-by: Dan Ryan <[email protected]>
@oz123 oz123 self-assigned this Aug 15, 2022
@oz123 oz123 mentioned this issue Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants