-
-
Notifications
You must be signed in to change notification settings - Fork 23
added uv and pnpm, updated readme #47
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job - cool to see this added. I left a few comments for your consideration. Please don't hesitate to let me know if you have any questions.
4. **PostgreSQL:** A running PostgreSQL database server (version 12+ recommended). You'll need the ability to create a database and a user. [Download PostgreSQL](https://www.postgresql.org/download/) | ||
5. **Redis:** A running Redis server. Celery uses this to manage background tasks. [Download Redis](https://redis.io/docs/getting-started/installation/) or use Docker. | ||
+6. **Node.js and npm:** For running the frontend application. Npm usually comes bundled with Node.js. [Download Node.js](https://nodejs.org/) (LTS version recommended). You can check your installation by running `node -v` and `npm -v` in your terminal. | ||
1. **Python:** Version 3.13 or higher. [Download Python](https://www.python.org/downloads/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to download Python (would uv
do this for us automatically)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, uv can manage install and manage python versions with uv install python 3.13
. This should be done after creating and activating the environment. You can use uv python list
to see what's installed and available.
Use uv to install setup virtual environment. | ||
|
||
```sh | ||
uv python install 3.13 # Install python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to explicitly install a Python version (would uv
select and install for us)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine if you have python already installed system-wide, the venv uv creates would just use that. However after many broken system configs, I avoid installing python directly and let uv manage it entirely. This also avoids some puzzling errors I've encountered which I now believe were caused by conflicts with the build-in pip that's installed with the official python installer.
Jon originally used 3.10, but running through with 3.13 alongside updating the libraries seemed to work.
That is a great point though! I'll update the readme to better detail installation of python with uv. Do you think a .python-version file would help? As outputted by uv python pin
* **On macOS/Linux:** | ||
|
||
```sh | ||
source .venv/bin/activate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to avoid this step by using uv run
when necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, using uv run
should walk up the directory path until it finds a pyproject.toml, and is simpler. I'll remove the second pyproject.toml here /MOSS/Older Experiments/pyproject.toml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, uv commands walk up the path tree until they hit a pyproject.toml , so it should work. I'll rename the other one at ./MOSS/Older Experiments/pyproject.toml
just in case.
Install all the required Python packages listed in `pyproject.toml`: | ||
|
||
```sh | ||
uv sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing as this step is typically automatic when using uv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea ok. Would it makes sense to split the readme into 2 sections? One for just getting the project running, and the other for developing.
1. **Terminal 1:** Backend API Server (`uvicorn backend.main:app ...`) | ||
2. **Terminal 2:** Celery Worker (`celery -A backend.celery_app worker ...`) | ||
3. **Terminal 3:** Frontend Development Server (`cd frontend && npm run dev`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably an opportunity to improve this by making the code here a run.sh
script (we have almost as much code as we do natural language in these lines). Depending on how you feel you could possibly add that to this PR or we could bump this into a new issue and work on it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too considered wrapping that in a script for this PR, but I wasn't sure whether to do it with a script and have different OS to consider/test, or add it to docker as well..
There is also the option of specifying it in pyproject.toml, which may be cleaner and closer aligned to a standardized approach. Here for instance, where they use taskipy
task runner to start the rest (ex. https://github.com/leosussan/fastapi-gino-arq-uvicorn/blob/master/pyproject.toml)
@@ -0,0 +1,27 @@ | |||
services: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to the top of the file to help describe what it does. We have that information within the readme but oftentimes when in development we are deep in the weeds of individual files, where it can help to have notes so as to not go searching for meaning elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is outside the bounds of this PR's focus. It might be a good idea for us to consider using Dependabot to help maintain the environment specifications for this project over time (esp. for Node and Python dependencies). uv
-style pyproject.toml
files aren't yet supported but likely will be in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using Dependabot is a great idea! we should create an issue for this so it doesn't get forgotten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it might have been a test file. Consider removing if we don't need it any longer. If we do still need it, consider placing in a source code directory where appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually the default file that is created with uv init
. The backend shouldn't serve that though, so we can remove it. Towards production we should add a 404 page and look into locking down the backend.
## Frontend Setup and Running | ||
```sh | ||
ur run uvicorn backend.main:app --reload --host 0.0.0.0 --port 8000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ur run uvicorn backend.main:app --reload --host 0.0.0.0 --port 8000 | |
uv run uvicorn backend.main:app --reload --host 0.0.0.0 --port 8000 |
npm run dev | ||
```sh | ||
pnpm dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying to run this I ran into the error: ERROR packages field missing or empty
. It's possible I did something wrong but I wanted to bring it up just in case there's a need to update the pnpm-workspace.yaml
or similar.
2. **Install uv** | ||
As described here: [uv - Installation](https://docs.astral.sh/uv/getting-started/installation/) | ||
|
||
```sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend adding more clear indication of which platform these commands are for (e.g. linux & mac vs windows)
Description
Updated the project to use the uv package package manager and its toolset to install python
Updated the project to use the pnpm package manager and its toolset to install node
Added docker for postgresql and redis
Fixes # (link issue)
Type of change