Refactor/cleanup and optimize codebase #41
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and simplifies the codebase by replacing the Node.js‐based frontend with a vanilla HTML/CSS/JavaScript implementation, removing database-backed storage in favor of session‐based in-memory storage, and enhancing security through updated Docker configurations and non-root user usage. Key changes include updates in session cookie handling and tests, consolidation of session management modules, comprehensive documentation updates, and CI/CD pipeline improvements.
Reviewed Changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/tests/unittest_session_manager.py | Updated tests to verify the new cookie httponly flag (set to False) reflecting API access changes. |
| app/tests/unittest_main_api.py | Modified error message text and endpoint responses for clarity. |
| app/tests/run_unittests.py | Adjusted test output status messages by removing symbolic indicators. |
| app/storage/session_manager*.py | Removed duplicate/backup session management modules to consolidate functionality. |
| app/routers/session_segmentation.py | Refactored segmentation workflow to use a unified image path construction function and added processing time tracking; adjusted exception handling. |
| app/routers/session_images.py | Removed legacy background segmentation tasks and updated session endpoints. |
| README.md | Updated documentation to reflect architecture simplification, session-based storage, and Docker changes. |
| Dockerfile.web & Dockerfile.app | Upgraded to Python 3.12-slim, added non-root users, and applied security updates in dependency installation. |
| .github/workflows/ci.yml | Revised CI steps to validate static files and check JavaScript syntax; updated dependencies installation. |
Comments suppressed due to low confidence (2)
app/routers/session_segmentation.py:151
- Consider raising an HTTPException with a status code (e.g., 400) instead of a ValueError to provide a clearer, API-consistent error response for clients.
raise ValueError("Could not generate polygon from mask")
app/routers/session_segmentation.py:125
- Ensure that the fallback call to set_image in the 'else' block is correctly indented to clearly associate it with the corresponding conditional branch.
height, width = segmenter.set_image(image_path)
- Add requirements-ci.txt with CPU version of PyTorch for CI environments - Update CI workflow to install PyTorch CPU version with correct index - Add SAM model installation to CI workflow - Update README.md with PyTorch version documentation - Add comments to requirements.txt explaining CUDA vs CPU versions Fixes CI failure where torch==2.5.1+cu121 was not available in GitHub Actions runners
- Added requirements-ci.txt to the project structure documentation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request includes changes to simplify the project architecture, enhance security, and improve documentation. Key updates involve replacing the frontend framework with a vanilla HTML/CSS/JavaScript implementation, transitioning from a database-backed storage system to session-based in-memory storage, and integrating security enhancements in Docker configurations.
Project architecture simplification:
Dockerfile.web: Replaced Node.js-based frontend with Python's built-in HTTP server to serve static files, removed npm dependencies, and added a non-root user for security.app/db/database.py: Removed database-related code, transitioning to session-based in-memory storage for simplicity.Security enhancements:
Dockerfile.app: Added a non-root user, improved package installation with--no-install-recommends, and upgraded Python to version 3.12 for backend container. [1] [2]Dockerfile.web: Added a non-root user and security updates for the frontend container.CI/CD pipeline updates:
.github/workflows/ci.yml: Replacedweb-buildjob withweb-validationto validate static files and JavaScript syntax, and updated Docker build job to include testing. [1] [2]Documentation improvements:
README.md: Updated to reflect architectural changes, added detailed project structure, API reference, and clarified Docker vs local development setup. [1] [2]Dependency updates:
Dockerfile.app: Upgraded PyTorch and other dependencies, and switched to usingrequirements.txtfor backend dependencies.