Skip to content

Conversation

romaingd
Copy link
Contributor

@romaingd romaingd commented Mar 7, 2025

Description of changes

Hi! This PR addresses #10554 by adding an entry for uv to the "Setting up a development environment" tab panel.

It also adds a new just sync <backend> recipe, where <backend> is optional, to make installing the adequate dependencies easier. Better names are of course welcome!

Issues closed

@github-actions github-actions bot added the docs Documentation related issues or PRs label Mar 7, 2025
@@ -3,7 +3,7 @@ COPY --from=ghcr.io/astral-sh/uv:0.6.4 /uv /uvx /bin/
ARG USERNAME=vscode

RUN apt-get update && \
apt-get install -y --no-install-recommends libgdal-dev && \
apt-get install -y --no-install-recommends libgdal-dev python3-dev && \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required to avoid issues with psycopg2 when installing all dependencies - see https://stackoverflow.com/questions/19843945/psycopg-python-h-no-such-file-or-directory

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see this as an inline comment in the dockerfile, IDK what @cpcloud thinks though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that'd be nice, but not a blocker!

1. Activate the virtual environment

```sh
source .venv/bin/activate
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't unfortunately be a just recipe - see https://byabbe.se/2024/02/21/using-python-virtual-environments-through-just

@romaingd romaingd marked this pull request as ready for review March 7, 2025 19:13
@romaingd
Copy link
Contributor Author

romaingd commented Mar 7, 2025

Thanks for the review! I've applied your suggestions for a simpler UI and adapted the PR header accordingly.


1. [Install `uv`](https://docs.astral.sh/uv/getting-started/installation/)

1. [Install `gh`](https://cli.github.com/manual/installation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is gh only needed for the gh repo fork --clone call below? Or do the other justfile recipes require it? If it is only for this first step, I could see it being nice to removing this as a dependency: only instructing people to install uv, and then providing the raw git commands (would also need to instruct them to fork the repo, probably in the github web ui).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to replace a one liner with clicking on a web page?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't mean to sound like a jerk there. After rereading what I wrote I realized it probably didn't come off exactly as I meant.

The original reason I chose gh is because it's the tool that lets you stay in one mode while setting everything up, whereas having people click to fork requires context switching. I don't see what we gain here by removing the gh dependency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no worrie! Sounds good to me, just wanted to throw it out there

@NickCrews
Copy link
Contributor

Thanks @romaingd! I just had a few optional nits, but in general looks great, LGTM.

@cpcloud cpcloud merged commit dd33f47 into ibis-project:main Mar 7, 2025
130 of 131 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Update dev docs per to describe using uv
3 participants