Update docs for current development installation #228
Conversation
WalkthroughThe changes modify development setup and documentation files. The Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new installation section assumes familiarity with the provided
docker-compose.yaml(profiles, service names, ports); consider briefly naming the relevant services and where the file lives so new contributors can more easily map the commands to the compose setup. - The production deployment note at the end of
installation.mdis quite vague; tightening this up with at least a minimal example of how to make the database persistent (e.g., pointing to the relevant volume configuration indocker-compose.yaml) would make the guidance more actionable even before full docs are written.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new installation section assumes familiarity with the provided `docker-compose.yaml` (profiles, service names, ports); consider briefly naming the relevant services and where the file lives so new contributors can more easily map the commands to the compose setup.
- The production deployment note at the end of `installation.md` is quite vague; tightening this up with at least a minimal example of how to make the database persistent (e.g., pointing to the relevant volume configuration in `docker-compose.yaml`) would make the guidance more actionable even before full docs are written.
## Individual Comments
### Comment 1
<location> `docs/installation.md:3` </location>
<code_context>
# Installation
-*Current instructions tested on Mac, but likely work on most Unix systems.*
+The primary way to run this service is through a docker container.
+The REST API needs to be able to connect to a MySQL database with the OpenML "openml" and "openml_expdb" databases.
+The `docker-compose.yaml` file of this project defines these together out of the box.
</code_context>
<issue_to_address>
**nitpick (typo):** Consider capitalizing "Docker" as it is a proper product name.
Use "Docker container" here to match the official product name and common usage.
```suggestion
The primary way to run this service is through a Docker container.
```
</issue_to_address>
### Comment 2
<location> `docs/installation.md:13` </location>
<code_context>
-??? tip "Use `pyenv` to manage Python installations"
+Once the containers are started, you can run tests with `docker exec -it openml-python-rest-api python -m pytest -m "not php_api" tests`.
+For migration testing, which compares output of the Python based REST API with the old PHP based one, also start the PHP server (`docker compose --profile "php" --profile "python" up -d`) and include tests with the `php_api` marker/fixture: `docker exec -it openml-python-rest-api python -m pytest tests`.
- We recommend using [`pyenv`](https://github.com/pyenv/pyenv) if you are working with
</code_context>
<issue_to_address>
**nitpick (typo):** Hyphenate "Python-based" (and optionally "PHP-based") for correct compound adjective usage.
This keeps the documentation grammatically correct and consistent in its use of compound adjectives.
```suggestion
For migration testing, which compares output of the Python-based REST API with the old PHP-based one, also start the PHP server (`docker compose --profile "php" --profile "python" up -d`) and include tests with the `php_api` marker/fixture: `docker exec -it openml-python-rest-api python -m pytest tests`.
```
</issue_to_address>
### Comment 3
<location> `docs/installation.md:17` </location>
<code_context>
- > pyenv local
- 3.12
- ```
+ The PHP REST API needs Elastic Search. In some cases, it also needs the ES indices to be built.
+ The current set up does not automatically build ES indices, because that takes a long time.
+ When we start testing more upload functionality, for which the PHP API needs built indices, we'll work on an ES image with prebuilt indices.
</code_context>
<issue_to_address>
**suggestion (typo):** The search engine name is usually spelled "Elasticsearch" as one word.
To keep terminology consistent with the official product name, please update occurrences of "Elastic Search" to "Elasticsearch".
```suggestion
The PHP REST API needs Elasticsearch. In some cases, it also needs the ES indices to be built.
```
</issue_to_address>
### Comment 4
<location> `docs/installation.md:21-22` </location>
<code_context>
-!!! Failure ""
-
- Instructions are incomplete. Please have patience while we are adding more documentation.
+Information for a production deployment will follow, in a nutshell you need to configure the REST API to connect to a persistent database,
+which can be the one defined in `docker-compose.yaml` if has an appropriately mounted volume.
</code_context>
<issue_to_address>
**issue (typo):** Missing "it" in "if has an appropriately mounted volume" and the sentence could be tightened.
This should read "if it has an appropriately mounted volume" to fix the missing subject. You could also split the long sentence for clarity, e.g.: "Information for a production deployment will follow. In a nutshell, you need to configure the REST API to connect to a persistent database, which can be the one defined in `docker-compose.yaml` if it has an appropriately mounted volume."
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Installation | ||
|
|
||
| *Current instructions tested on Mac, but likely work on most Unix systems.* | ||
| The primary way to run this service is through a docker container. |
There was a problem hiding this comment.
nitpick (typo): Consider capitalizing "Docker" as it is a proper product name.
Use "Docker container" here to match the official product name and common usage.
| The primary way to run this service is through a docker container. | |
| The primary way to run this service is through a Docker container. |
|
|
||
| ??? tip "Use `pyenv` to manage Python installations" | ||
| Once the containers are started, you can run tests with `docker exec -it openml-python-rest-api python -m pytest -m "not php_api" tests`. | ||
| For migration testing, which compares output of the Python based REST API with the old PHP based one, also start the PHP server (`docker compose --profile "php" --profile "python" up -d`) and include tests with the `php_api` marker/fixture: `docker exec -it openml-python-rest-api python -m pytest tests`. |
There was a problem hiding this comment.
nitpick (typo): Hyphenate "Python-based" (and optionally "PHP-based") for correct compound adjective usage.
This keeps the documentation grammatically correct and consistent in its use of compound adjectives.
| For migration testing, which compares output of the Python based REST API with the old PHP based one, also start the PHP server (`docker compose --profile "php" --profile "python" up -d`) and include tests with the `php_api` marker/fixture: `docker exec -it openml-python-rest-api python -m pytest tests`. | |
| For migration testing, which compares output of the Python-based REST API with the old PHP-based one, also start the PHP server (`docker compose --profile "php" --profile "python" up -d`) and include tests with the `php_api` marker/fixture: `docker exec -it openml-python-rest-api python -m pytest tests`. |
| > pyenv local | ||
| 3.12 | ||
| ``` | ||
| The PHP REST API needs Elastic Search. In some cases, it also needs the ES indices to be built. |
There was a problem hiding this comment.
suggestion (typo): The search engine name is usually spelled "Elasticsearch" as one word.
To keep terminology consistent with the official product name, please update occurrences of "Elastic Search" to "Elasticsearch".
| The PHP REST API needs Elastic Search. In some cases, it also needs the ES indices to be built. | |
| The PHP REST API needs Elasticsearch. In some cases, it also needs the ES indices to be built. |
| Information for a production deployment will follow, in a nutshell you need to configure the REST API to connect to a persistent database, | ||
| which can be the one defined in `docker-compose.yaml` if has an appropriately mounted volume. |
There was a problem hiding this comment.
issue (typo): Missing "it" in "if has an appropriately mounted volume" and the sentence could be tightened.
This should read "if it has an appropriately mounted volume" to fix the missing subject. You could also split the long sentence for clarity, e.g.: "Information for a production deployment will follow. In a nutshell, you need to configure the REST API to connect to a persistent database, which can be the one defined in docker-compose.yaml if it has an appropriately mounted volume."
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/installation.md (2)
9-10: Wrap the bare URL in markdown link syntax.Line 10 contains a bare URL which should be wrapped in markdown syntax for better rendering and accessibility.
🔎 Proposed fix
-The REST API will be exposed on port 8001 on the host machine. To visit the Swagger Docs, visit http://localhost:8001/docs. +The REST API will be exposed on port 8001 on the host machine. To visit the Swagger Docs, visit [http://localhost:8001/docs](http://localhost:8001/docs).
12-13: Add hyphens to compound adjectives.Line 13 uses "Python based" and "PHP based" where hyphenated compound adjectives are required by English grammar when modifying nouns.
🔎 Proposed fix
-For migration testing, which compares output of the Python based REST API with the old PHP based one, also start the PHP server... +For migration testing, which compares output of the Python-based REST API with the old PHP-based one, also start the PHP server...
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.python-versiondocs/installation.mddocs/migration.md
💤 Files with no reviewable changes (1)
- .python-version
🧰 Additional context used
🪛 LanguageTool
docs/installation.md
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...ing, which compares output of the Python based REST API with the old PHP based on...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...e Python based REST API with the old PHP based one, also start the PHP server (`d...
(QB_NEW_EN_HYPHEN)
[style] ~14-~14: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1468 characters long)
Context: ...thon-rest-api python -m pytest tests`. !!! note The PHP REST API needs Elasti...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/installation.md
10-10: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (4)
docs/migration.md (1)
16-17: Clear and timely caveat for readers.The added paragraph appropriately qualifies the migration guide and sets clear expectations about documentation updates before production deployment.
docs/installation.md (3)
3-7: Effective Docker-first introduction.The rewritten introduction clearly positions containerized deployment as the primary approach and appropriately documents key requirements and development-specific caveats.
15-19: Helpful context for PHP testing setup.The note appropriately explains why Elasticsearch indices are not auto-built and sets expectations for future improvements as upload functionality is tested.
21-22: Appropriate deferral of production guidance.The brief note appropriately defers production deployment details to a future section while providing minimal orientation for those who need it.
I removed the local installation instructions. While it is still possible, it is not recommended, and I want to avoid having to spend time troubleshooting local setups.