-
Notifications
You must be signed in to change notification settings - Fork 180
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
Add uv support #1518
base: main
Are you sure you want to change the base?
Add uv support #1518
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1518 +/- ##
===========================================
- Coverage 89.70% 79.33% -10.38%
===========================================
Files 122 125 +3
Lines 9394 9808 +414
===========================================
- Hits 8427 7781 -646
- Misses 967 2027 +1060
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
looks good, thanks @abelaba !
I added a couple of comments, some of them are questions for clarification, so it's all open for discussion.
```bash | ||
uv venv | ||
``` | ||
|
||
Then from `sbi` folder activate the virtual enviroment by running: | ||
|
||
- For `macOS` or `Linux` users | ||
```bash | ||
source .venv/bin/activate | ||
``` | ||
|
||
- For `Windows` users | ||
```bash | ||
.venv\Scripts\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.
I could be missing something, but wouldn't the user need to run something like uv add sbi
?
or how would sbi
be installed here?
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 section is for creating and activating the virtual environment only.
```bash | ||
uv venv | ||
``` | ||
|
||
Then from `sbi` folder activate the virtual enviroment by running: | ||
|
||
- For `macOS` or `Linux` users | ||
```bash | ||
source .venv/bin/activate | ||
``` | ||
|
||
- For `Windows` users | ||
```bash | ||
.venv\Scripts\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.
since we are in the developer setting, I suggest one would need to something like:
cd sbi
uv init
uv sync --all-extras
oruv sync --extra dev
and this would install all dependencies?
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 didn't add the
cd sbi
command as contributors would already be in the sbi folder on Step 3. - The uv init command is used for creating a new project. Running that command gives the following error.
error: Project is already initialized in '...\sbi' ('pyproject.toml\' file exists)
- I was following the pip interface and instead of the
uv sync
command I was using theuv venv
command which creates the virtual environment.
```bash | ||
uv pip install -e ".[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.
I see you are using the uv-pip
interface here. hm, not sure. I think it would be better to explicitly use uv
(the interface to pip
is just for convenience).
see comment above, which actually relates to this part here.
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.
Thank you for all the comments. Okay, I will update the documentation to use uv
explicitly instead of the pip interface.
```bash | ||
uv pip install pre-commit | ||
pre-commit install | ||
``` |
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 pre-commit
should be included in the uv sync --all-extras
because it is listed in the dev
dependencies. Thus, just pre-commit install
should suffice here. but please test it locally.
If you are using [`uv`](http://docs.astral.sh/uv/) you can install the documentation dependencies using: | ||
|
||
```bash | ||
uv pip install -e ".[doc]" |
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 would be uv sync --extra doc
This PR updates the documentation and readme file to support working with uv.
Fixes #1401
Bullet points from the parent issue