Updates to README to be more accurate. #267
Conversation
|
@igiloh-pinecone Review? |
README.md
Outdated
| @@ -258,6 +258,8 @@ To run the canopy server for production, please run: | |||
| gunicorn canopy_server.app:app --worker-class uvicorn.workers.UvicornWorker --bind 0.0.0.0:PORT --workers WORKER_COUNT | |||
| ``` | |||
|
|
|||
| Note: replace `PORT` and `WORKER_COUNT` with values. | |||
There was a problem hiding this comment.
No, no, that's actually a typo 🤦 .
This was supposed to be $PORT and $WORKER_COUNT. And I guess these come from our docker image, where we explicitly set these env vars (with default, even if the user hasn't provided them).
Can you fix the typo please?
There was a problem hiding this comment.
Actually, I'm not even sure why @miararoy has changed the main README to use these env vars - unlike in the docker case, if someone just pip install canopy, he is not guaranteed to have those env vars at all. I think we should just revert to what we had before:
--bind 0.0.0.0:8000 --workers <number of desired worker processes>
There was a problem hiding this comment.
@aulorbe all in all LGTM.
Please see my comment about that PORT and WORKER_COUNT thing. I think we should just go back to the original phrasing, and then we don't need more explanations later.
Changed the production recommendation - docker first, running gunicorn as alternative
Finalized reversing docker and gunicorn order
Problem
Quick updates to README to reflect current state of things.
Solution
Describe the approach you took. Link to any relevant bugs, issues, docs, or other resources.
Type of Change
Test Plan
Describe specific steps for validating this change.