Rework Installation Instructions #5015
Conversation
- Add some structure to the texts regarding the operating system, Python interpreter, and virtual environment - Reformat end user and developer installation headings to use markdown ATX-style headers - Extend instructions to use a virtual environment like recommended in the text
| cd /path/to/your/parlai-app | ||
| python3.8 -m venv venv | ||
| venv/bin/pip install --upgrade pip setuptools wheel | ||
| venv/bin/pip install parlai |
There was a problem hiding this comment.
This looks unnecessarily complex when all we are asking is for a pip install
There was a problem hiding this comment.
Since the virtual environment never gets activated (e.g. source venv/bin/activate in Bash), pip install would call the system pip (e.g. /usr/bin/pipunder Ubuntu). The readme specifically recommends using the virtual environment, so I specify using the executables installed into it by addressing them with their full path. Note that this works in any shell on any unixoid operating system. In particular, these instructions are portable between at least many shells. Using relative paths seems more compact to me than to list commands for every supported shell, like bash, csh and fish.
| cd ~/ParlAI | ||
| python3.8 -m venv venv | ||
| venv/bin/pip install --upgrade pip setuptools wheel | ||
| venv/bin/python setup.py develop |
There was a problem hiding this comment.
ditto here; does this fail if you do not specify venv/bin/python?
There was a problem hiding this comment.
Again, python is the system Python interpreter, because the venv was never activated, which I suppose is the more portable option. Consider that users of the readme are usually copying these commands, so there is a tradeoff to be made between the legibility of single commands and readability of overall the instruction document. I mean, if there were options listed for many shells, users could easily get confused. This portable version should work for all the supported shells and is still obvious enough, I feel.
|
@klshuster Thanks for having a look at this. I would be glad to contribute. I was in need of better installation instructions, and I hope I can help the next person in my position. |
|
Could you please rebase onto the main branch? then I think we can go ahead and merge this |
|
This PR has not had activity in 30 days. Closing due to staleness. |
Patch description
Testing steps
Other information
These fixes are largely stylistic, but the instructions are now more complete and actually work.