CHORE: Replaced Flask with Haystack - #2
Conversation
…ognized due to phoenix not fully starting up
…healthchecks. Updated some readmes to remove references to flask
…s is FastAPI based-- to create a /health endpoint in hopes of passing stability check when deploying to AWS
Co-authored-by: Kevin Boyer <kevinboyer@navapbc.com>
Co-authored-by: Kevin Boyer <kevinboyer@navapbc.com>
michelle-hadfield-nava
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR replaces Flask/APIFlask with Haystack as the core framework, transitioning from a general-purpose web API to a specialized AI/ML pipeline framework.
Key Changes:
- Completely replaced Flask/APIFlask with Haystack and Hayhooks for building AI pipelines
- Removed all Flask-specific functionality including user management, database layers, and authentication
- Added sample Haystack pipeline infrastructure with pipeline wrapper and YAML configuration
Reviewed Changes
Copilot reviewed 65 out of 73 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/app.py | Core application replaced with Hayhooks app instead of Flask |
| app/pyproject.toml | Dependencies updated to use Haystack ecosystem instead of Flask |
| app/src/pipelines/ | New directory structure for Haystack AI pipelines |
| infra/modules/service/main.tf | Infrastructure change to disable read-only root filesystem |
| docs/ | Documentation and decision records removed (no longer relevant) |
Comments suppressed due to low confidence (1)
app/src/app_config.py:9
- Outdated comment references Flask documentation which is no longer relevant since the application now uses Hayhooks instead of Flask.
host: str = "127.0.0.1"
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Yoom Lam <yoom@navapbc.com>
Co-authored-by: Yoom Lam <yoom@navapbc.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
KevinJBoyer
left a comment
KevinJBoyer
left a comment
There was a problem hiding this comment.
Nice! Looks like there a few linter / formatting / test errors to clean up, but we're almost to the finish line on this one!
| args: | ||
| - RUN_UID=${RUN_UID:-4000} | ||
| - RUN_USER=${RUN_USER:-app} | ||
| command: ["poetry", "run", "flask", "--app", "src.app", "run", "--host", "0.0.0.0", "--port", "8080", "--reload"] |
There was a problem hiding this comment.
for my own understanding, does this work because the Dockerfile has the appropriate command already?
There was a problem hiding this comment.
Yes, should we have the command here are well?
There was a problem hiding this comment.
It seems like the main difference is that the docker-compose.yml (which is only used for local development) includes a --reload for hot reloading the app when the source files change, but I couldn't find out how to enable the same in hayhooks
There was a problem hiding this comment.
I haven't come across hotloading for hayhooks. I believe hayhooks, upon startup, loads and precompiles the Python files. One can add or remove pipelines using the hayhooks API, but hotloading for an existing pipeline is not yet a feature.
…mmands left over from troubleshooting, linted
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the Flask web framework with Haystack, transitioning from a Flask-based API application to a Haystack-based LLM application framework. The change removes the existing Flask API infrastructure and implements a new Haystack pipeline system for building applications powered by large language models.
- Removed Flask API endpoints, authentication, and database integration components
- Added Haystack framework dependencies and pipeline configuration
- Updated infrastructure to support the new Haystack application architecture
Reviewed Changes
Copilot reviewed 49 out of 58 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| infra/modules/service/main.tf | Updated ECS task definition to disable read-only root filesystem |
| docs/app/technical-overview.md | Replaced Flask documentation with Haystack framework description |
| docs/app/api-details.md | Removed entire Flask API documentation file |
| app/src/app.py | Replaced Flask application with Hayhooks application and health endpoint |
| app/pyproject.toml | Updated dependencies from Flask to Haystack and changed Python version |
| app/src/pipelines/sample_pipeline/ | Added new Haystack pipeline implementation |
| Multiple test files | Removed Flask-specific test files and configurations |
| Multiple service files | Removed user management service layer files |
| Multiple API files | Removed Flask API routes, schemas, and blueprints |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| before passing the request to the route handler. | ||
| In the `api_key` security scheme, the `X-Auth` points to the | ||
| function that is run to do the authentication. | ||
| TODO update |
There was a problem hiding this comment.
The authentication section contains a placeholder 'TODO update' comment. This should be updated with proper documentation for how authentication works in the new Haystack application, or the section should be removed if authentication is not applicable.
| TODO update | |
| n/a - Authentication is not yet implemented for this API. |
| - RUN_UID=${RUN_UID:-4000} | ||
| - RUN_USER=${RUN_USER:-app} | ||
| env_file: local.env | ||
| # TODO - Update this to work with Haystack |
There was a problem hiding this comment.
The debug configuration contains a TODO comment indicating it needs to be updated for Haystack. This should be addressed to ensure debugging functionality works with the new framework.
| networkMode = "awsvpc", | ||
| essential = true, | ||
| readonlyRootFilesystem = !var.enable_command_execution, | ||
| readonlyRootFilesystem = false, |
There was a problem hiding this comment.
Setting readonlyRootFilesystem to false reduces container security by allowing the container to write to its root filesystem. This should be justified or reverted to true with proper volume mounts for writable directories.
| readonlyRootFilesystem = false, | |
| readonlyRootFilesystem = true, |
Co-authored-by: Kevin Boyer <kevinboyer@navapbc.com>
Co-authored-by: Kevin Boyer <kevinboyer@navapbc.com>
There was a problem hiding this comment.
Ah -- it's the addition of app/__init__.py that's causing the issue. Delete this file (and I think you should restore app/src/api/__init__.py) and the linter runs correctly
Co-authored-by: Kevin Boyer <kevinboyer@navapbc.com>
KevinJBoyer
left a comment
KevinJBoyer
left a comment
There was a problem hiding this comment.
Nice work! To keep this sprint unblocked, we'll address the package updates / vulnerability scans in a tech debt PR :)
| # DOCKLE_ACCEPT_FILES="file1,path/to/file2,file3/path,etc" | ||
|
|
||
| # The apiflask/settings file is a stub file that apiflask creates, and has no sensitive data in. We are ignoring it since it is unused | ||
| DOCKLE_ACCEPT_FILES=app/.venv/lib/python3.13/site-packages/apiflask/settings.py |
There was a problem hiding this comment.
There was an earlier version of this PR that ignored the haystack file, you could bring that change back in before merging, or we can bring it in to the tech debt PR
Ticket
https://navalabs.atlassian.net/browse/DST-1106
Changes
2)Add Haystack
Context for reviewers
TBD
Testing
TBD
Preview environment for app
♻️ Environment destroyed ♻️