Skip to content

Refactor Docker setup and enhance API documentation#17

Open
artaasd95 wants to merge 1 commit into
lynote-ai:mainfrom
artaasd95:add-fastapi-support
Open

Refactor Docker setup and enhance API documentation#17
artaasd95 wants to merge 1 commit into
lynote-ai:mainfrom
artaasd95:add-fastapi-support

Conversation

@artaasd95

Copy link
Copy Markdown
Contributor
  • Updated docker-compose.yml to rename services and add CLI support.
  • Introduced Dockerfiles for API and CLI services.
  • Added FastAPI server implementation for the Standard Pipeline.
  • Enhanced README.md and api-reference.md with usage instructions for the new REST API.
  • Updated requirements.txt and setup.py to include FastAPI and related dependencies.
  • Added tests for the API endpoints to ensure functionality.

- Updated docker-compose.yml to rename services and add CLI support.
- Introduced Dockerfiles for API and CLI services.
- Added FastAPI server implementation for the Standard Pipeline.
- Enhanced README.md and api-reference.md with usage instructions for the new REST API.
- Updated requirements.txt and setup.py to include FastAPI and related dependencies.
- Added tests for the API endpoints to ensure functionality.

@molly554 molly554 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution! Adding a REST API for the Standard Pipeline is a useful feature. However, there are a few issues that need to be addressed before merging.

❌ 2 Test Failures

test_humanize_returns_result and test_humanize_include_steps both return 503 instead of 200.

Root cause: The client fixture sets app.state.config directly, but TestClient(app) triggers the lifespan context manager which reads the real config/config.toml (doesn't exist in CI/test environments) and overwrites app.state.config = None. The fixture assignment happens before TestClient.__enter__, so lifespan clobbers it.

Fix: Either mock _load_config / _validate_config inside the fixture, or use app.dependency_overrides to inject test config, or restructure lifespan to skip validation when config is already set.

⚠️ _validate_config() Hardcodes DeepSeek

if not api_keys.get("deepseek_api_key"):
    missing.append("deepseek_api_key")

This conflicts with PR #16 (OpenRouter support). When a user configures [llm].provider = "openrouter" with openrouter_api_key, this validation will incorrectly report deepseek_api_key as missing.

Fix: Use provider-aware validation — check the key that matches the configured [llm].provider, or reuse resolve_llm_config() from llm_client.py which already handles this correctly.

⚠️ fastapi/uvicorn/pydantic Should Be Optional Dependencies

These 3 packages are added to the base requirements.txt and install_requires, meaning every CLI-only user must install them. The API server is an optional feature.

Suggested fix: Move them to extras_require:

extras_require={
    "api": [
        "fastapi>=0.109.0",
        "uvicorn[standard]>=0.27.0",
        "pydantic>=2.0.0",
    ],
    "legacy": [...],
},

And in api.py / pipeline.py --serve, catch ImportError with a helpful message like "pip install humanize-text[api]".

💡 Minor Suggestions (non-blocking)

  1. --serve binds 0.0.0.0 — This exposes the API to the entire local network. Consider defaulting to 127.0.0.1 (localhost only) and adding a --host option for those who need external access.

  2. No authentication on /humanize — Anyone who can reach port 8000 can consume the user's DeepSeek/Niutrans API quota. Consider adding at least a simple bearer token check (configurable via env var).


To summarize — please fix the 2 failing tests, make config validation provider-aware, and move the API deps to extras. Happy to re-review after those changes. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants