Update docs for current development installation #228
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe 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.
![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.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/installation.md (1)
21-22: Fix grammar and sentence structure in production deployment note.The sentence is missing the subject "it" in the conditional clause and has awkward punctuation. This issue was flagged in a previous review and needs to be corrected.
🔎 Proposed fix for grammar and clarity
- 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. + 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.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/installation.md
🧰 Additional context used
🪛 LanguageTool
docs/installation.md
[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 (3)
docs/installation.md (3)
3-8: Clear Docker-first introduction.The introduction effectively explains the Docker-based approach and sets appropriate expectations about the development database behavior.
12-13: Clear test instructions for both scenarios.The testing commands properly distinguish between standard and migration testing, providing developers with clear guidance.
15-19: Important Elasticsearch context properly documented.The note effectively communicates the Elasticsearch requirement and current limitations. The MkDocs admonition syntax is appropriate here.
| For development, it should suffice to run the services from a fresh clone by running `docker compose --profile "python" up -d`. | ||
| The REST API will be exposed on port 8001 on the host machine. To visit the Swagger Docs, visit http://localhost:8001/docs. |
There was a problem hiding this comment.
Format bare URL as a markdown link per MD034.
Line 10 contains a bare URL that should be formatted as a markdown link to comply with markdown linting standards.
🔎 Proposed fix to format the URL as a markdown link
- 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).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| For development, it should suffice to run the services from a fresh clone by running `docker compose --profile "python" up -d`. | |
| The REST API will be exposed on port 8001 on the host machine. To visit the Swagger Docs, visit http://localhost:8001/docs. | |
| For development, it should suffice to run the services from a fresh clone by running `docker compose --profile "python" up -d`. | |
| 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). |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
10-10: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In docs/installation.md around lines 9 to 10, there's a bare URL
(http://localhost:8001/docs) on line 10 that violates MD034; replace the bare
URL with a Markdown link (for example: "To visit the Swagger Docs, visit
[Swagger Docs](http://localhost:8001/docs).") so the URL is formatted as link
text rather than a bare URL.
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.