Skip to content

Install package locally #76

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.

Sign up for GitHub

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

Open
codeinthehole wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Install package locally #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a2c8a0
Introduce a `makefile` for local-dev installation
codeinthehole Oct 11, 2024
ba7688e
Install package locally
codeinthehole Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,18 @@ source venv/bin/activate
You could also use [virtualenvwrapper], [direnv] or any similar tool
to help manage your virtual environments.

### Install the Python dependencies
### Install the `timezone_tools` package and its dependencies

To install all the development dependencies in your virtual environment,
To install the `timezone_tools` package and all the development dependencies in your virtual environment,
run:

```sh
python3 -m pip install -r requirements.txt
make dev
```

[direnv]: https://direnv.net
[virtualenvwrapper]: https://virtualenvwrapper.readthedocs.io/

The tools used to manage this project will be installed.

### Run the tests

To start the tests with [tox](https://tox.wiki), run:
Expand All @@ -48,10 +46,10 @@ tox
```

Alternatively, if you want to run the tests directly in your virtual environment,
you many run the tests with:
you may run the tests with:

```sh
PYTHONPATH=src python3 -m pytest
python3 -m pytest
```

### Static analysis
Expand Down
11 changes: 11 additions & 0 deletions makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
SHELL=/bin/bash


.PHONY:dev
dev: install_python_packages


.PHONY:install_python_packages
install_python_packages: requirements.txt
# Install the `timezone_tools` package (in editable mode) and the development dependencies.
python3 -m pip install -r requirements.txt -e .

@codeinthehole codeinthehole Oct 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Am not wedded to having a makefile if you'd prefer to have this command in CONTRIBUTING.md instead. But I think it's probably a good idea to keep commonly-used commands in a makefile (or invoke tasks if we get there) to make contributors' lives easier. Especially once the command has a few options in it.

@samueljsb samueljsb Oct 14, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can change the command in the contributing guide -- I have no particular objection to that.

Although, I'm not sure I understand why everybody should need to install -e .. That does seem like something that's needed for a specific personal workflow. (again, not getting in your way there)

I would object to using GNU make as a task runner, though. I think it makes it less obvious what's going on, because contributors need to dig through the Makefile and it's funny syntax (what does "phony" mean? what is the indirection doing here? etc) to understand that the requirements are just in requirements.txt and if they have a preferred tool for managing their virtual environments, they just need to install that.

@marcelofern marcelofern Oct 14, 2024 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure I understand why everybody should need to install -e ..

To trace a parallel... When I am working on django-pg-migration-tools I can use nox to test. However, it takes too much time to run nox as it goes through the test matrix for all the permutations of Django and Python. It takes a lot of time even in parallel as I have more permutations than cores to run them.

The alternative is to run python -m pytest using the virtual environment on my local machine (no testing matrix, thus faster). It's nice to be able to do so when I am experimenting and want a quick feedback. Without installing the package in edit mode I'd have to do a pip install . every time I change a file I'm experimenting with before I run tests.

To trace a parallel...

That said, I am not sure what the current situation for this package is. It looks smaller from a testing matrix perspective and number of tests - CI takes 5s to run the testing matrix. If that is tolerable then devs could run that locally and it might not be that big of a deal.

@samueljsb samueljsb Oct 14, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it takes too much time to run nox

That isn't the case for this project:

$ time tox -p
py310: OK ✔ in 1.29 seconds
py311: OK ✔ in 1.36 seconds
py313: OK ✔ in 1.37 seconds
py312: OK ✔ in 1.41 seconds
  py310: OK (1.29=setup[0.90]+cmd[0.40] seconds)
  py311: OK (1.36=setup[0.89]+cmd[0.47] seconds)
  py312: OK (1.41=setup[0.88]+cmd[0.52] seconds)
  py313: OK (1.37=setup[0.89]+cmd[0.48] seconds)
  report: OK (0.18=setup[0.01]+cmd[0.08,0.09] seconds)
  congratulations :) (1.63 seconds)
tox -p  1.88s user 0.70s system 145% cpu 1.778 total

Even without -p:

tox  1.63s user 0.52s system 81% cpu 2.626 total

Without installing the package in edit mode I'd have to do a pip install . every time I change a file

Oh, if we're installing the project then I understand why we'd want an editable install, rather than a static one, I just don't understand why it would be necessary to install it at all.

@codeinthehole codeinthehole Oct 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Although, I'm not sure I understand why everybody should need to install -e .. That does seem like something that's needed for a specific personal workflow. (again, not getting in your way there)

I suppose this boils down to whether running pytest in the local (non-tox) virtualenv is a common or recommended practice that we should make possible with the standard installation process.

My instinct is that running pytest in this way is a more common practice than doing everything via tox.

While tox is fast in a small repo like this, it's still significantly slower. On my machine pytest takes 200-300ms to run the test suite while tox -e py312 takes 1-2 seconds. That's noticeable when doing TDD.

But crucially, does it do harm to have the package installed in editable mode?

It doesn't get in the way of people who only want to run tests via tox right? But not doing it is inconvenient for people who want to use the local pytest approach.

Plus it's inline with our how our CookieCutter repos are set-up, lowering the barrier to contributing to this repo for other devs.

That's what I think we should install the package locally.