- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Comments
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. |
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. |
@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 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. |
Regarding #77
|
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. |
Ok, so I just installed on macOS Catalina via pip and for me the path to the executable is |
This is the case if you do it with the python interpreter which comes with the system. This is how it looks for me on Mojave. 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. |
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. |
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 |
I think this might be congruent with the experience from #94 (comment). |
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) |
With #120, we test that pre-commit installed with pip on linux works. In general, we also search |
I'd still favor a more lightweight approach than conda as the recommended way, e.g. via 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
Maybe
|
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:
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:
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 I hope this makes sense. |
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. |
If you use Thinking about this more, I am not sure if the whole effort of trying to help users here is worth 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. 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 :) I think that's all my thoughts for now - curious how you'll do it in the end 👍 |
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 |
Sorry, a little late to the party, but #97 (and relying on Sys.which()) works for me. Thanks for all of the investment here! |
Currently, the package throws an error if the user installed
pre-commit
with pip.The text was updated successfully, but these errors were encountered: