-
Notifications
You must be signed in to change notification settings - Fork 846
π¦ feat(inspect): Add docker build #3136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/geti-inspect
Are you sure you want to change the base?
π¦ feat(inspect): Add docker build #3136
Conversation
Mainly based on the UI files and architecture from [Geti Tune](https://github.com/open-edge-platform/training_extensions/tree/develop/ui)
* initial backend commit Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * app -> src Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * Remove empty file Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * move code Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix style backend Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * rename media endpoint Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> --------- Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com>
β¦#2942) add unit tests for endpoints Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com>
These won't be needed yet
β¦tform#2948) * Use src folder inside run.sh * Set openapi_url path * Update to react 19 * Specify bash language in readme
β¦edge-platform#2945) * add training + inference endpoint Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * remove model api Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * cleanup code Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * update async execution Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * improve training worker loop and predict endpoint Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix style Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix style to use python3.10 generics Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add tests for services Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * style fix Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * style fix Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * style fix Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * style fix Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * style fix Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> --------- Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com>
β¦edge-platform#2961) * Add github actions for ui and server of geti inspect * Exclude UI from pre-commit prettier configuration The UI uses a different prettier configuration that does not seem to be picked up by pre-commit. * Add newline to .prettierignore * Apply prettier to `geti-inspect.yaml` * Generate OpenAPI spec before running UI checks * Checkout with lfs * Fix lint issues by removing wip components * Try installing git lfs in the playwright docker image * Fix unused noqa
β¦edge-platform#2963) * Add OpenAPI route * Remove MSW browser worker * Rename infernece to inspect * Update navbar title * Rename infernece to inspect
* rename models/ to pydantic_models/ Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * switch to use async session context manager Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix unit tests Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add pipeline endpoints Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix frame aquisition worker and rename pipiline endpoints Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add sources and sinks endpoints Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * stream loading working Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add webrtc endpoints Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix workers: stream loading + inference + dispatcher Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix sinks Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * style Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add unit tests and address comments Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add tests Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * fix example schema Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add todo Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> --------- Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com>
β¦2970) * chore: Add path alias to icons * feat: Add sidebar with dataset, models and stats
* feat: Display placeholders for images * refactor: Fix scrollbar * chore: Remove learn more
* chore(inspect): Update UI scripts * chore: Update github actions * chore: Update port to 8000
β¦en-edge-platform#2978) chore(inspect): Configure project(s) mocks for playwright
* chore: Add project route * feat: Add project management * revert ui lock change * chore: Remove not needed code for project management
β¦ts from the UI (open-edge-platform#2980) chore(inspect): Update allowed origins
β¦ training progress (open-edge-platform#2984) * feat: Allow user to upload images * feat: List uploaded images * chore: Extract components to separate files and add ready to train and training progress * chore: comment thumbnail url generation
chore(inspect): Update openapi page title
β¦en-edge-platform#2989) * chore(inspect): Renamed app to application * chore(inspect): Rename app to application in github actions
β¦of email (open-edge-platform#2990) refactor: Update photo placeholder to use indicator instead of email
β¦dge-platform#2991) Improve error and suspense handling in router By moving all of the routes into a single root route we can make sure that all routes are rendered inside of an layout that has a suspense and error boundary.
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
β¦open-edge-platform#2992) refactor: Improvements to the jobs management and training
β¦platform#2994) chore(inspect): Add more models
π fix(inspect): Fix unable to start train job
chore(inspect): Update uv.lock
* add trainable models endpoint * fix Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add test Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add copyright Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> --------- Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com>
β¦rm#3004) * add thumbnails endpoint Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * add tests Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * generate thumbnails as background task Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> * update docstring Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com> --------- Signed-off-by: Ma, Xiangxiang <xiangxiang.ma@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 26 out of 29 changed files in this pull request and generated 2 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "http://localhost:9000", | ||
| "http://127.0.0.1:9000", | ||
| ], | ||
| allow_origins=["*"], |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing CORS from specific origins to allow all origins (*) introduces a security risk. This allows any website to make requests to your API. Consider using environment variables to configure allowed origins for different deployment environments, or maintain the original restrictive list for production.
| rev: v2.12.0.2 | ||
| hooks: | ||
| - id: hadolint | ||
| files: ^application/\.packaging/docker/ # TODO: this will break when the Dockerfile is moved |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment indicates technical debt. Consider using a more flexible file pattern (e.g., Dockerfile$ or **/*Dockerfile*) that won't break if the Dockerfile is moved, or create a tracking issue for this concern rather than leaving it as an inline TODO.
| files: ^application/\.packaging/docker/ # TODO: this will break when the Dockerfile is moved | |
| files: Dockerfile$|Dockerfile\..*$ |
| WORKDIR /app | ||
|
|
||
| RUN --mount=type=cache,target=/home/non-root/.cache/uv,uid=10001,gid=10001 \ | ||
| uv sync --frozen --no-dev --extra cpu --extra application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why run uv sync from the root anomalib folder instead of application?
Installing inspect as an extra might be useful during development but I don't see the value in production environments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which pre-commit config did you run to result in all these style fixes? Using prek, ruff does not apply these changes.
application/backend/pyproject.toml
Outdated
| [tool.hatch.build.targets.wheel.sources] | ||
| "src" = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "src" = "" do?
* Replace "pre-commit" with "prek" in both root and application/backend pyproject.toml files. * Remove dependency for "geti-inspect" in the root pyproject.toml. Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 3 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "http://localhost:9000", | ||
| "http://127.0.0.1:9000", | ||
| ], | ||
| allow_origins=["*"], |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing all origins with allow_origins=['*'] creates a security vulnerability by permitting cross-origin requests from any domain. This bypasses CORS protection and could enable malicious sites to access the API. Restrict allow_origins to specific trusted domains or use environment variables to configure allowed origins.
|
|
||
| ```bash | ||
| cd application/.packaging/docker | ||
| AI_DEVICE=cuda docker compose up |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The device name 'cuda' is inconsistent with the docker-compose.yml and Dockerfile which use 'cu124'. Either update the example to use 'cu124' or ensure all references use 'cuda' consistently.
| AI_DEVICE=cuda docker compose up | |
| AI_DEVICE=cu124 docker compose up |
application/backend/pyproject.toml
Outdated
| "paho-mqtt~=2.1.0", | ||
| ] | ||
|
|
||
| # Explicit versions are needed as we can't propogate extra dependencies to anomalib |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'propogate' to 'propagate'.
| # Explicit versions are needed as we can't propogate extra dependencies to anomalib | |
| # Explicit versions are needed as we can't propagate extra dependencies to anomalib |
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 1 comment.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Map all host devices to provide access to webcams and other attached devices | ||
| privileged: true | ||
| devices: | ||
| - /dev:/dev |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the container in privileged mode with full access to /dev is a significant security risk that grants the container nearly unrestricted access to the host system. Consider mapping only specific required devices (e.g., /dev/video0 for webcams) and removing privileged: true unless absolutely necessary for the application's core functionality.
| # Map all host devices to provide access to webcams and other attached devices | |
| privileged: true | |
| devices: | |
| - /dev:/dev | |
| # Map only required host devices (e.g., webcam) to the container for security. | |
| # privileged: true # Removed for security; only enable if absolutely necessary. | |
| # devices: | |
| # - /dev/video0:/dev/video0 # Example: map only webcam device if needed. |
| "http://localhost:9000", | ||
| "http://127.0.0.1:9000", | ||
| ], | ||
| allow_origins=["*"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather implement it via env variable: https://github.com/open-edge-platform/geti-prompt/blob/main/application/backend/app/main.py#L73
| # Alembic | ||
| alembic_config_path: str = "src/alembic.ini" | ||
| alembic_script_location: str = "src/alembic" | ||
| alembic_config_path: str = str(_MODULE_DIR / "alembic.ini") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be adjusted further when we introduce Pyinstaller
| @@ -0,0 +1,52 @@ | |||
| # PID file in a location non-root user can write to | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we omit having nginx by serving UI static using FastAPI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: why put everything in a parent folder .packaging/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move it into application/docker (without .packaging?) no need to hide it imho. This would also align well with the other applications.
|
|
||
| EXPOSE 80 | ||
|
|
||
| CMD ["sh", "-c", "nginx && exec uv run src/main.py"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use backend/run.sh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add example of running the container with camera passthrough? I guess would be a common usecase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to build it and it works. However, couldn't get camera passthrough to work on mac. Have you tested it on linux?
Signed-off-by: Ashwin Vaidya <ashwin.vaidya@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 14 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
application/docker/Dockerfile:1
- The dockerfile path references
application/.packaging/docker/Dockerfile, but based on the file structure, the Dockerfile is located atapplication/docker/Dockerfile. This mismatch will cause the build to fail.
#------------------------------------------
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## To create CPU build | ||
|
|
||
| ```bash | ||
| cd application/.packaging/docker |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory path in the instructions is incorrect. The actual docker files are in application/docker/, not application/.packaging/docker/. This inconsistency appears in all three build instruction sections and will cause users to encounter errors when following the documentation.
|
|
||
|
|
||
| @webui_router.get("/", include_in_schema=False) | ||
| async def get_webui(full_path: str = "") -> FileResponse: # noqa: ARG001 |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function parameter full_path is declared but never used, and there's no logic to prevent potential path traversal attacks or handle different routes. If this endpoint is meant to handle multiple paths, the implementation should use full_path to serve the appropriate files. If it only serves index.html, the parameter should be removed.
| async def get_webui(full_path: str = "") -> FileResponse: # noqa: ARG001 | |
| async def get_webui() -> FileResponse: |
| @webui_router.get("/", include_in_schema=False) | ||
| async def get_webui(full_path: str = "") -> FileResponse: # noqa: ARG001 | ||
| """Get the webui index.html file.""" | ||
| if settings.static_files_dir and not (file_path := Path(settings.static_files_dir) / "index.html").exists(): |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If settings.static_files_dir is None or empty, file_path will not be defined, causing an UnboundLocalError on line 20. The condition should ensure file_path is always defined before the return statement, or raise an appropriate error when static_files_dir is not configured.
| if settings.static_files_dir and not (file_path := Path(settings.static_files_dir) / "index.html").exists(): | |
| if not settings.static_files_dir: | |
| raise HTTPException(status_code=500, detail="Static files directory is not configured") | |
| file_path = Path(settings.static_files_dir) / "index.html" | |
| if not file_path.exists(): |
| "http://localhost:9000", | ||
| "http://127.0.0.1:9000", | ||
| ], | ||
| allow_origins=["*"], |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing all origins with allow_origins=[\"*\"] is a security risk in production environments as it permits any domain to make requests to the API. Consider making this configurable through settings and restricting it to specific trusted origins in production.
| && echo "deb [arch=amd64,i386 signed-by=/usr/share/keyrings/intel-graphics.gpg] https://repositories.intel.com/gpu/ubuntu jammy unified" | \ | ||
| tee /etc/apt/sources.list.d/intel-gpu-jammy.list \ |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XPU build stage is adding Ubuntu 'jammy' repositories to a Debian-based image (python:3.13-slim is based on Debian). This repository mismatch may cause package installation issues or incompatibilities. Use the appropriate Debian-compatible Intel GPU repository instead.
| && echo "deb [arch=amd64,i386 signed-by=/usr/share/keyrings/intel-graphics.gpg] https://repositories.intel.com/gpu/ubuntu jammy unified" | \ | |
| tee /etc/apt/sources.list.d/intel-gpu-jammy.list \ | |
| && echo "deb [arch=amd64,i386 signed-by=/usr/share/keyrings/intel-graphics.gpg] https://repositories.intel.com/gpu/debian bookworm unified" | \ | |
| tee /etc/apt/sources.list.d/intel-gpu-bookworm.list \ |
Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
π Description
TODO
β¨ Changes
Select what type of change your PR is:
β Checklist
Before you submit your pull request, please make sure you have completed the following steps:
For more information about code review checklists, see the Code Review Checklist.