internetarchive / internetarchive/openlibrary
Eliminate `_init_path` (and consolidate around `Makefile`)?
- Dominant language
- Python
- Stars
- 6.7k
- Forks
- 2k
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 138
Description
This is a note to explore using the `Makefile` and setting `PYTHONPATH`:
- as a way to avoid relying on `scripts/_init_path.py` to set `PYTHONPATH`, as `_init_path` causes problems when trying to import the scripts for unit testing (though these problem can be overcome, e.g. with `patch`);
- to standardize the process for interacting with the containers for common tasks;
- to avoid having to create 'special cases' for installing packages via `pip` as `root` so that root can run, e.g., scripts depending on `web.py` and `babel`.
### Describe the problem that you'd like solved
As mentioned, `_init_path` can cause import woes, we sometimes tell people to use `make` for common tasks e.g. `make test-py` and other times we tell people to run scripts e.g., `./scripts/i18n-messages add ` or `./scripts/copydocs.py`, and some of these scripts require adding packages in [`Dockerfile` outside of `requirements.txt`](https://github.com/internetarchive/openlibrary/blob/fae5a4487261e4d8a5033c3456d9cfca215285df/docker/Dockerfile.oldev#L14), solely so `root` can run some scripts.
### Proposal & Constraints
We'd have to both (1) look at the implications and (2) think about whether this is even the right approach, but what about putting the script commands we tell people to run into the `Makefile`, and then using the `openlibrary` account's Python packages within the container? There are some questions here such as:
- does this even work/what does it break;
- how much do we want to run in containers (e.g. should the `make` command itself be executed in the container, or should the `Makefile` be updated to itself execute `docker compose run`;
- what does this do for `pre-commit`;
- what does this to for GitHub Actions; and
- what does this to for scripts run from `cron`?
Here's a quick experimental patch that made me think this is worth a bit more thought, even if we ultimately decide it's not the way forward:
```diff
diff --git a/Makefile b/Makefile
index 350b65fa5..7300e9b7e 100644
--- a/Makefile
+++ b/Makefile
@@ -10,6 +10,8 @@ OSP_DUMP_LOCATION=/solr-updater-data/osp_totals.db
# Use python from local env if it exists or else default to python in the path.
PYTHON=$(if $(wildcard env),env/bin/python,python)
+# ???
+PYTHON_VERSION = $(shell python -c 'import sys; print(f"{sys.version_info.major}.{sys.version_info.minor}")')
.PHONY: all clean distclean git css js components i18n lint
@@ -38,7 +40,7 @@ components: $(COMPONENTS_DIR)/*.vue
::: $^
i18n:
- $(PYTHON) ./scripts/i18n-messages compile
+ PYTHONPATH="/home/openlibrary/.local/lib/python$(PYTHON_VERSION)/site-packages:$(PYTHONPATH)" ./scripts/i18n-messages $(cmd)
git:
git submodule init
diff --git a/scripts/i18n-messages b/scripts/i18n-messages
index 98f28e5d6..a83ee50a8 100755
--- a/scripts/i18n-messages
+++ b/scripts/i18n-messages
@@ -2,7 +2,6 @@
"""Utility script to extract all translatable messages from templates and
macros and write to openlibrary/i18n/messages.pot file.
"""
-import _init_path # noqa: F401 Imported for its side effect of setting PYTHONPATH
import sys
from openlibrary import i18n
```
The above would be run with `docker compose run -uroot home make i18n cmd=compile` (but perhaps it could be changed to be `make i18n cmd=compile` with the `Makefile` calling `docker compose run [...]`).
### Additional context
### Stakeholders
Contributor guide
Research direction
Start by reading the Makefile, scripts/_init_path.py, scripts/i18n-messages, and the referenced docker/Dockerfile.oldev section. Map how commands are run locally, in containers, by pre-commit, GitHub Actions, and cron. Done means the project has a decided approach for Python paths and common commands, with the affected workflows validated.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- docker-compose, python
- Domain
- build-system, devops, tooling
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100