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

Rework Ubuntu Installation Instructions #99

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Bengt
Copy link

@Bengt Bengt commented Apr 18, 2023

  • Be explicit about Ubuntu version numbers
  • Clone repository first to keep the venv local
  • Be explicit about Python version numbers
  • Prefer using apt instead of apt-get
  • Add --yes flag to apt install subcommand
  • Use a virtual environment to not pollute the system
  • Install python packages in one command so that pip can resolve their dependencies properly
  • Use python executables from virtual environment for running analyse.py script

Bengt and others added 3 commits April 18, 2023 13:34
- Be explicit about Ubuntu version numbers
- Clone repository first to keep the venv local
- Be explicit about Python version numbers
- Prefer using apt instead of apt-get
- Add --yes flag to apt install subcommand
- Use a virtual environment to not pollute the system
- Install python packages in one command so that pip can resolve their dependencies properly
- Use python executables from virtual environment for running analyse.py script
Add Nvidia LibCUDNN package to virtual environment for a roughtly 3x speedup
@kahst
Copy link
Owner

kahst commented Apr 19, 2023

Thanks. To be honest, I personally don't like to put virtual environments in the readme, because it complicates things and doesn't look as clean as without. I understand the motivation, I just don't like it as brief overview of the packages we use. I agree with putting everything in one line and cloning first though. I'd like to have a discussion about virtual environments in the readme to get to know peoples opinions.

@fegue
Copy link
Collaborator

fegue commented Apr 19, 2023

In my opinion, the Pros outweigh the Cons.
Sure, the instructions are more complicated, and less technical people might find it off-putting.
But that's exactly when you should use venvs. It's much more painful to deal with different numpy versions than to follow these simple instructions.

@Bengt
Copy link
Author

Bengt commented Apr 20, 2023

@kahst thanks for picking some of my ideas up for inclusion in the main branch.

Virtual Environments

@kahst could please elaborate on your preference to not virtual environments into readmes? I noticed that there are commands for creating conda environments in the readme. While I agree that these are not technically virtual environments, their commands are more verbose, and they do not solve any additional problems. Are you okay with these? What is your opinion/policy here? Consider:

conda create -n birdnet-analyzer python=3.10 -c conda-forge -y

vs.

python3.8 -m venv venv

Clean Readme

I suppose, the cleanest readme would only list a docker run command that implicitly pulls a predefined image containing the project code and its runtime environment. That would however hide a lot of interesting details and make working with the project code more difficult than necessary. Instead, each readme needs to strike a balance between conciseness and showing something actually useful to its readers. My aim in writing readmes has been that users can follow them from top to bottom and by that arrive at a workable setup in some interesting sense. I have found that I can arrive at a sensible measure of verbosity by considering if it is easier to name a prerequisite or to show how to fulfill it. In the case of the virtual environment, one could name the prerequisite:

The follwing instructions assume that you run them in a virtual environment created by the venv module of Python 3.8 and that you have activated said environment.

I mean, that is certainly doable and arguable also readable to users that are familiar with venv. However, as @fegue pointed out, many users are not that well versed in setting up virtual environments. I would argue that making users aware of the benefis of virtual environments and expecting them to get familiar with the accompanying tools is out of scope for this readme. Instead, the focus should lay on setting up this project to use it in an application or develop this project further. Therefore, I think that the readme should rather to give a recommendation on using a virtual environment and show how to set up and use one, without any discussion. Yes, there alternatives to using venv like conda, which is already mentioned in other sections of the readme. However, discussing their pros and cons is clearly out of scope for this readme. Moreso, one could even drop the recommendation and just assume that everyone will be using venv. That might seem arrogant, but it makes the readme more concise and is based on the recommendation of the Python project:

[pip and venv] [...} are recommended if higher-level tools do not suit your needs.

https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/#creating-a-virtual-environment

@fegue
Copy link
Collaborator

fegue commented Apr 24, 2023

The conda part is on me because apple needs some dependencies from their channel

conda install -c apple tensorflow-deps

@LimitlessGreen
Copy link
Contributor

If you're using conda, you should consider recommending the C++ implementation mamba instead.
Dependent on the environment, conda can take ages to solve. I changed to mamba a half year ago and does not regret it.

@LimitlessGreen
Copy link
Contributor

To be honest, I personally don't like to put virtual environments in the readme, because it complicates things and doesn't look as clean as without.

Sounds to me that it's time to start a wiki. Let me know, I can put this together :octocat:

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.

4 participants