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

Look for pip installations of pre-commit #78

Closed
michaelquinn32 opened this issue Dec 19, 2019 · 18 comments
Closed

Look for pip installations of pre-commit #78

michaelquinn32 opened this issue Dec 19, 2019 · 18 comments

Comments

@michaelquinn32
Copy link
Contributor

Currently, the package throws an error if the user installed pre-commit with pip.

@pat-s
Copy link
Contributor

pat-s commented Dec 19, 2019

I think this could maybe even be the default? As long as the python pkgs are in $PATH I'd expect the pkg to work OOB?

Also I am still not sure if conda is the best choice as the default installer suggestion? I talked to two people and they were like 😱 when I told them that it needs/suggests conda as a dep.

@lorenzwalthert
Copy link
Owner

Can you @michaelquinn32 please specify which error appears? Please provide a reprex or similar (i.e. what command produced which error) for the sake of documentation and so we can discuss how to fix it.

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Dec 20, 2019

@pat-s maybe you are right and I should change my mind and we should discuss what's the appropriate default installer. My first goal was simply to provide a way to install precommit from R. The idea was to use conda because that way, I saw how to handle everything from R. Also, you can now even install conda with reticulate using conda_install(). I am not sure this is possible with pip. My understanding is that everyone who uses the reticulate package also needs conda. Can you confirm that?

However, this issue (and the related PR) do not address the installation process as such if I understand @michaelquinn32 correctly. The goal is only to make sure the pre-commit executable is found if it was previously installed with pip instead of conda. This would make the linking of the executable redundant, not more and not less. Correct @michaelquinn32? We can later think about if we can use pip from R and which should be the default installer.

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Dec 20, 2019

Regarding #77

  • Does this suggestion work across platforms? I am not sure pip installs into the same directory for all operating systems. According to this post, it rather seems local, not .local. Can you check this?
  • We should check for both pip install precommit --user as well as pip install precommit.
  • Where do pip3 installations go? To the same place as pip?

@michaelquinn32
Copy link
Contributor Author

It will be more complicated if users set up their own virtualenvs or do other things to configure where binaries go, but pip by default installs exectuable scripts to $HOME/.local/bin.

https://www.python.org/dev/peps/pep-0370/

I don't think its version dependent.

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Dec 21, 2019

Ok, so I just installed on macOS Catalina via pip and for me the path to the executable is $HOME/Library/Python/3.7/bin/pre-commit. This makes things more complicated for us since the path is version dependent. Also, I don't get a difference between pip3 and pip.

@pat-s
Copy link
Contributor

pat-s commented Dec 21, 2019

This is the case if you do it with the python interpreter which comes with the system.
Many people on macOS have python installed via homebrew.

This is how it looks for me on Mojave.
Python packages can be in a lot of places and it depends on which interpreter you use.
I am not having much knowledge when it comes to Python but it feels there should be a more easy way than accounting manually for all of this.
Otherwise it becomes a mess across OS and versions.

System python (/usr/bin/python)

import site
site.getsitepackages()

['/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages', '/System/Library/Frameworks/Python.framework/Versions/2.7/lib/site-python', '/Library/Python/2.7/site-packages']

homebrew python (/usr/local/bin/python)

import site
site.getsitepackages()

['/usr/local/Cellar/python@2/2.7.17/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages', '/usr/local/Cellar/python@2/2.7.17/Frameworks/Python.framework/Versions/2.7/lib/site-python']

Apart from letting the user specify the location of the executable via an env var/options, there should be a canonical way how this is done? I do not know it but it seems this is a thing that many people depending on python packages have to deal with?

I'll ask around a bit if some people with more knowledge in python have an idea.

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Dec 21, 2019

I agree many people have to deal with this, so as @pat-s mentioned in a private conversation, RStudio seems to favour the solution presented in their reticulate 1.14 post (published yesterday), which is via Miniconda. I think we should eventually follow this path.

Also thanks for your clarification @pat-s. The thing is that precommit (this package) currently uses the bash executable pre-commit and not module in python. Maybe we should change that and use the python module from R. I think this is the usual use case for reticulate.

Edit: I don't think the upstream framework pre-commit is designed to be used from Python. I think the fact that it is written in Python is an implementation detail, bash is the exposed API.

@lorenzwalthert
Copy link
Owner

I'd rather not prefer to add a uncertain / hacky solution here unless someone is willing to provide a PR and expand tests accordingly on all platforms. I believe any user who installs via pip or home-brew can also $where pre-commit and link the executable via the R option in the .Rprofile.

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Feb 24, 2020

[...] As long as the python pkgs are in $PATH I'd expect the pkg to work OOB?

I think this might be congruent with the experience from #94 (comment).

@lorenzwalthert
Copy link
Owner

In fact no. I think we should first make sure executables that are on the PATH also work without explicit linking with an R option. See #94 (comment)

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Mar 28, 2020

With #120, we test that pre-commit installed with pip on linux works. In general, we also search $PATH. @michaelquinn32 do you want if it works now for you?

@pat-s
Copy link
Contributor

pat-s commented Mar 28, 2020

I'd still favor a more lightweight approach than conda as the recommended way, e.g. via pip3. Looks like the latter is just the alternative now and users are pushed towards using conda. Even miniconda is conda (and with that a completely now package manager which could be avoided).
pip is always present when python is installed whereas conda is not.
Therefore I'd vote for pip - also because conda has higher potential to interfere with R packages etc.

It is of course your decision in the end what to choose / recommend, these are just 2 cents 😃

I once said I'll think about an approach that I'd recommend for all of this. I party forgot about it but here we go:

Suggestion

  • macOS: Use brew install pre-commit (easily autodetectable in usr/local/bin)
  • Linux: Differs between distros. On Ubuntu apt recommends to use snap via snap install pre-commit with the exec at /snap/bin/pre-commit. However, this gets you only v1.15.2. Doing a simple pip3 install -U pre-commit gets you the latest version. The exec is stored in ~/.local/bin/pre-commit.
  • Windows: The most popular package manager is "chocolatey" but it does not offer pre-commit. Here I would also just go for py -m pip install -U pre-commit. Then - well, I ran out of time to find the location where python puts its exec files of python packages 😆 (but that should not to hard to find out).

Maybe precommit::set_env() would be simpler than precommit::path_pre_commit_exec()?

precommit::set_env() could browse these default paths depending on what Sys.info[["sysname"]] returns and with this cover 90% of all users.
If no installation is detected it can suggest what to do, conditioned on the OS.

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Mar 28, 2020

Thanks @pat-s, I appreciate that you took time to investigate this. In my latest comment, I was just referring to the reason this issue was opened, which was that pip installations were not detected. This should be resolved now for Linux. For macOS, the installation directory is Python version dependent. I get this warning:

WARNING: The scripts pre-commit, pre-commit-validate-config and pre-commit-validate-manifest are installed in '/Users/lorenz/Library/Python/3.7/bin' which is not on PATH.
Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location

Together with the installation instruction for the manual installation, the average user should be able to link the executable. I am happy to accept PRs (with tests) to auto-detect executables on more platforms when installed with different installation methods. The locations you provided above are a good starting point for this. We should also document this somewhere. If anyone want's to work on that, please open a new issue.


As far as the default installation method goes, my initial thoughts were:

  • give the user as much flexibility in the installation method they choose. If the installation method they choose puts the executable in a place that is on the $PATH, they don't have to do anything further. Otherwise, they must manually link the executable with the R option precommit.executable. The effort is minimal.

  • make very easy to handle everything from R, cross-plattform. This is the case with conda and many people are already familiar with it. If a user thinks the conda dependency is too heavy or unnecessary, he is most likely advanced enough to link the executable from any custom installation method. The other ones probably are fine with conda.

I have no objection to the installation methods you provided above. If people don't want to use conda, they can use their installation method of choice and in case the executable is not on the $PATH, they link it once.
What I don't want to do is to provide an API in {precommit} to install software from R via brew or any other package manager. I believe that's beyond the scope of this package, in particular because it's platform dependent, platform version dependent, installation method dependent. I envision {precommit} is a lightweight wrapper and hook repository for R hooks. Also, I don't want to offer functionality I cannot test and maintain easily.

I hope this makes sense.

@lorenzwalthert
Copy link
Owner

Also I adapted the README a bit recently to give people more of a choice by explicitly stating that conda is not the only option. I am open for suggestions on improving this further.

@pat-s
Copy link
Contributor

pat-s commented Mar 29, 2020

For macOS, the installation directory is Python version dependent.

If you use brew which is the de-facto package manager on macOS then it will always be in /usr/local/bin.

Thinking about this more, I am not sure if the whole effort of trying to help users here is worth it.
This package just wraps a system library like so many others do. Usually such packages (e.g. {git2r} or {curl}) do not provide install instructions or functions for that. During install they try to detect a valid exec in the PATH and if not found tell the user to install it.

Often you get some hints how these libs can be installed with the local package managers on Ubuntu/macOS but otherwise users are on their own.
I think you could also do it like that for precommit: Stating the dependency that is needed and don't worry more about it.

Recommending an external package manager like conda, which is wrapped into a function of the package, can surely be helpful for some users but also feels kind of unusual :)
And its comes with lots of time investment and potential headaches 😅

I think that's all my thoughts for now - curious how you'll do it in the end 👍

@lorenzwalthert
Copy link
Owner

I opened #128 to track the progress of auto-detecting from officially supported installation methods.

I agree that usually, installation methods are not tided to the R package. So once we detect all methods in #128, we can maybe refactor the docs a bit to reflect that. I am not afraid that conda installation is going to be the root of much headache because RStudio backs it and they will ensure it will continue to work. Also, {precommit} does not install conda by default. It just installs into a conda environment if there is one. The reason why I am going into this direction at all is because I want to lower the barrier to use {precommit} as much as possible. A lot of R users don't even know what $PATH is, so I want the package to work out of the box for these people too.

@michaelquinn32
Copy link
Contributor Author

Sorry, a little late to the party, but #97 (and relying on Sys.which()) works for me. Thanks for all of the investment here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants