diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 05c4dd81b7..f1f93bec0d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -17,7 +17,6 @@ OpenVINO Model Server (OVMS) is a high-performance inference serving platform bu - `third_party/` — Third-party dependency definitions for Bazel - `Dockerfile.ubuntu` / `Dockerfile.redhat` — Multi-stage Dockerfiles for Linux builds - `Makefile` — Orchestrates Docker-based builds and test runs -- `*.bat` files — Windows build and setup scripts ## Code Style @@ -25,167 +24,41 @@ OpenVINO Model Server (OVMS) is a high-performance inference serving platform bu - Run `make style` to check formatting - Apache 2.0 license headers are required on all source files -## Expertise Areas - -1. **OpenVINO Expertise:** - - Proficient with OpenVINO core libraries and `ov::genai` components - - Familiar with OpenVINO performance optimization techniques -2. **C++ Proficiency:** - - Strong C++17 skills - - Familiar with best practices in memory management, concurrency, and template programming -3. **Serving Infrastructure:** - - gRPC and REST API handler design - - Model management, pipeline orchestration, and MediaPipe integration - - C API (`libovms_shared.so`) surface and bindings -4. **Build System Awareness:** - - Bazel build configuration and dependency management - - Minimizing C++ build times: forward declarations, include-what-you-use, avoiding transitive header leakage - - Understanding of Bazel targets, build flags (`--//:distro`), and multi-stage Docker builds - ## Code Review Instructions for PRs When analyzing a Pull Request, follow this protocol: +### C++ Guidelines & Performance + 1. Follow **C++ Core Guidelines** strictly. Include references in review comments. 2. Check for **hidden performance costs**: avoid `dynamic_cast` on the hot path; suggest `static_cast` or redesign if the type is known. 3. **Avoid copies**: ensure large data structures (tensors, buffers) are passed by reference or moved, not copied. -4. **Documentation**: ensure new public APIs have docstrings in C++ headers and Python bindings; update `docs/` as needed. -5. **Test coverage**: ensure that new features or changes have corresponding tests in `src/test/`. -6. **Formatting & safety:** - - No `using namespace std; using namespace ov;`. Prefer explicit using with specific symbols if needed, for readability. - - No `auto` for primitive types where it obscures readability. - - Use `const` and `constexpr` wherever possible. -7. Pass non-fundamental values by `const` reference wherever possible. -8. Prefer member initializer lists over direct assignments in constructor bodies. -9. Verify that the result of every newly introduced function is used in at least one call site (except `void` functions). -10. Use descriptive function and variable names. Avoid duplicate code — extract common functionality into reusable utilities. -11. When initial container values are known upfront, prefer initializer-list / brace-initialization over constructing an empty container and inserting. -12. Unused functions and includes are not allowed. Build times are already long — do not add unnecessary `#include` directives. Prefer forward declarations where possible and follow the include-what-you-use principle. - - **Forward-declare in headers, include in `.cpp`**: if a header only uses pointers or references to a type, use a forward declaration (`class Foo;`) instead of `#include "foo.hpp"`. Move the full `#include` to the `.cpp` file where the type is actually used. - - **Keep headers self-contained but minimal**: each header must compile on its own, but should not pull in transitive dependencies that callers don't need. - - **Prefer opaque types / Pimpl**: for complex implementation details, consider the Pimpl idiom to keep implementation-only types out of the public header entirely. - - **Never include a header solely for a typedef or enum**: forward-declare the enum (`enum class Foo;` in C++17) or relocate the typedef to a lightweight `fwd.hpp`-style header. -13. Be mindful when accepting `const T&` in constructors or functions that store the reference: verify that the referenced object's lifetime outlives the usage to avoid dangling references. - -## Build System - -### Bazel (primary build tool) - -Building and testing is done **inside a Docker `-build` container** with the repository mounted. Developers do not run Bazel on the host directly. - -**Important:** Building the `-build` image from scratch is time-expensive, but may be required if dependencies or the build environment change. Before building a new one, check if one already exists: -```bash -docker images | grep -- -build -``` -If a `-build` image exists, start a container from it with the repository mounted: -```bash -docker run -it -v $(pwd):/ovms \ - -e http_proxy=$http_proxy -e https_proxy=$https_proxy -e no_proxy=$no_proxy \ - bash -``` -If a container from a previous session is still available (`docker ps -a`), reuse it with `docker start -i ` or `docker exec -it bash`. - -**Key Bazel targets:** - -| Target | Description | -|--------|-------------| -| `//src:ovms` | Main OVMS server binary | -| `//src:ovms_test` | C++ unit tests (gtest) | -| `//src:ovms_shared` | C API shared library (`libovms_shared.so`) | - -**Build the server:** -```bash -bazel build //src:ovms -``` - -**Build and run unit tests:** -```bash -bazel build //src:ovms_test -bazel test --test_summary=detailed --test_output=streamed //src:ovms_test -``` - -### Red Hat builds — distro flag - -For Red Hat (UBI9) builds, the `--//:distro=redhat` flag must be passed to all Bazel commands: -```bash -bazel build --//:distro=redhat //src:ovms -bazel test --//:distro=redhat //src:ovms_test -``` - -The default distro is `ubuntu`. Inside the `-build` container, the distro is auto-detected from `/etc/redhat-release`. +4. Pass non-fundamental values by `const` reference wherever possible. +5. Use `const` and `constexpr` wherever possible. +6. Prefer member initializer lists over direct assignments in constructor bodies. -### Makefile (Docker-based workflow) +### Naming, Structure & Duplication -The Makefile orchestrates full Docker-based builds. Key targets: - -| Target | Description | -|--------|-------------| -| `make docker_build` | Full build: builder image → package → release images (default target) | -| `make ovms_builder_image` | Build the `-build` Docker image (compilation container) | -| `make targz_package` | Extract `.tar.gz` release package | -| `make ovms_release_images` | Build CPU and GPU release Docker images | -| `make run_unit_tests` | Run C++ unit tests in the `-build` container | -| `make test_functional` | Run Python functional tests | -| `make style` / `make cpplint` | Code style checks | - -**Red Hat build via Make:** -```bash -make docker_build BASE_OS=redhat -``` - -**Supported `BASE_OS` values:** `ubuntu24`, `ubuntu22`, `redhat` - -### Dockerfile stages - -Both `Dockerfile.ubuntu` and `Dockerfile.redhat` use multi-stage builds: - -| Stage | Purpose | -|-------|---------| -| `base_build` | System dependencies, Boost, Azure SDK, OpenCV | -| `build` | Bazel + OpenVINO setup, compiles OVMS (the `-build` container) | -| `capi-build` | Builds C API shared library and examples | -| `pkg` | Packages everything into a `.tar.gz` | -| `release` | Minimal runtime image with entrypoint | - -### Windows builds - -Windows builds use batch files in the repository root: -- `windows_install_build_dependencies.bat` — Install build dependencies (MSVC 2022 Build Tools, etc.) -- `windows_build.bat` — Main build script -- `windows_test.bat` — Run tests - -Windows-specific Bazel config: `--config=win_mp_on_py_off` (or `--config=win_mp_on_py_on` for Python support). - -## Testing - -### Test setup - -Before running tests, test models must be prepared: -```bash -make prepare_models -``` +7. Use descriptive function and variable names. Avoid duplicate code — extract common functionality into reusable utilities. +8. When initial container values are known upfront, prefer initializer-list / brace-initialization over constructing an empty container and inserting. +9. Verify that the result of every newly introduced function is used in at least one call site (except `void` functions). +10. No `using namespace std; using namespace ov;`. Prefer explicit using with specific symbols if needed. +11. No `auto` for primitive types where it obscures readability. -Models are exported using the `demos/common/export_models/export_model.py` script (used internally by the test setup). +### Includes & Build Time -If LLM test models change (e.g., new model version, OpenVINO version change or config update), you may need to remove and regenerate the LLM test data: -```bash -rm -rf src/test/llm_testing -make prepare_models -``` - -### Running tests - -Running the full `//src:ovms_test` suite is **time-consuming**. During development, always run only the test fixtures relevant to your changes first using `--test_filter`: -```bash -bazel test --test_summary=detailed --test_output=streamed --test_filter="SuiteName.TestName" //src:ovms_test -``` -Run the full test suite only after the targeted tests pass. +12. **Unused functions and includes are not allowed.** Build times are already long — do not add unnecessary `#include` directives. + - **Ban umbrella includes in headers** (`openvino/openvino.hpp`, `rapidjson/document.h`, etc.). Include only the specific subheader you use. + - **Forward-declare in headers, include in `.cpp`**: if a header only uses pointers or references to a type, use a forward declaration (`class Foo;`) instead of `#include "foo.hpp"`. + - **Keep headers self-contained but minimal**: each header must compile on its own, but should not pull in transitive dependencies that callers don't need. + - **Never include a header solely for a typedef or enum**: forward-declare the enum (`enum class Foo;`) or relocate the typedef to a lightweight `fwd.hpp`-style header. + - **Prefer opaque types / Pimpl**: for complex implementation details, consider the Pimpl idiom to keep implementation-only types out of the public header. +13. **Every new `.cpp` file must belong to its own `ovms_cc_library` target** in a BUILD file. Do not add sources to monolithic targets. +14. **Cross-module coupling goes through interfaces**, not concrete classes. Expose narrow interfaces (`ModelInstanceProvider`, `ServableMetadataProvider`, `ServableNameChecker`) that return plain data and hide lifecycle objects inside implementations. -### Test structure +### Safety & Testing -- **Unit tests** are in `src/test/` — gtest-based C++ tests covering all server components -- Test files follow the `*_test.cpp` naming convention -- Test utilities: `test_utils.hpp`, `light_test_utils.hpp`, `c_api_test_utils.hpp` -- Test models are stored in `src/test/` subdirectories (e.g., `dummy/`, `passthrough/`, `summator/`) -- Specialized test areas: `src/test/llm/`, `src/test/mediapipe/`, `src/test/python/`, `src/test/embeddings/` +15. Be mindful when accepting `const T&` in constructors or functions that store the reference: verify that the referenced object's lifetime outlives the usage to avoid dangling references. +16. **Test coverage**: ensure that new features or changes have corresponding tests in `src/test/`. +17. **Documentation**: ensure new public APIs have docstrings in C++ headers and Python bindings; update `docs/` as needed. diff --git a/.github/instructions/bazel-build.instructions.md b/.github/instructions/bazel-build.instructions.md new file mode 100644 index 0000000000..c9d57f6fef --- /dev/null +++ b/.github/instructions/bazel-build.instructions.md @@ -0,0 +1,33 @@ +--- +applyTo: "**/{BUILD,BUILD.bazel}" +--- +# Bazel BUILD File Guidelines (OVMS) + +## Target Granularity + +- **One logical unit per `ovms_cc_library` target.** Each `.cpp` file should belong to its own target with its associated `.hpp` in `hdrs`. +- **Do not add sources to monolithic targets.** If you're tempted to add a file to an existing large target, create a new focused target instead. + +## Macros + +- Use `ovms_cc_library` for production code (sets standard copts, linkopts, local_defines). +- Use `ovms_test_cc_library` for test utility libraries (adds gtest dep automatically). +- Use `ovms_cc_test` for test binaries. +- Use `additional_copts` for feature-flag copts (`COPTS_MEDIAPIPE`, `COPTS_PYTHON`). Do not override `copts` directly. + +## Dependencies + +- **Include-what-you-use for deps**: list only direct dependencies. Do not rely on transitive deps. +- **Prefer narrow deps**: depend on interface targets (`servable_metadata_provider`, `model_instance_provider`) over heavy implementation targets (`modelmanager`) when possible. +- **Forward-declaration targets**: for lightweight `_fwd.hpp` headers, create a separate header-only target (e.g., `tensorinfo_fwd`) so consumers can depend on it without pulling the full implementation. +- **Use `select()` for optional deps**: MediaPipe, Python, Drogon features should be behind `select()` on the appropriate config setting. + +## Visibility + +- Default to no `visibility` (package-private). Only add `visibility = ["//visibility:public"]` when the target is genuinely needed outside its package. +- Tight visibility prevents accidental coupling between subsystems. + +## Naming Conventions + +- Target names: lowercase with descriptive names matching the source (e.g., `grpcservermodule`, `kfs_grpc_frontend`). +- Test targets: match the test file name pattern (e.g., `ensemble_tests` for `ensemble_tests.cpp`). diff --git a/.github/instructions/build-workflow.instructions.md b/.github/instructions/build-workflow.instructions.md new file mode 100644 index 0000000000..ad8e256543 --- /dev/null +++ b/.github/instructions/build-workflow.instructions.md @@ -0,0 +1,172 @@ +--- +description: "Build workflow, Docker setup, Makefile targets, style checks and test execution for OVMS" +--- +# OVMS Build & Test Workflow + +## Docker-Based Development + +Building and testing is done **inside a Docker `-build` container** with the repository mounted. Developers do not run Bazel on the host directly. + +### Checking for existing build images + +Before building a new image (which is time-expensive), check if one exists: +```bash +docker images | grep -- -build +``` + +### Finding a container with the current workspace mounted + +**Always start here** — search for a running container that already mounts the current repo: +```bash +docker ps -q | xargs -I{} docker inspect {} --format '{{.ID}} {{range .Mounts}}{{.Source}}{{end}}' | grep "$(pwd)" +``` +If found, use `docker exec -it bash` to enter it directly. + +If no running container matches, check stopped containers too: +```bash +docker ps -aq | xargs -I{} docker inspect {} --format '{{.ID}} {{range .Mounts}}{{.Source}}{{end}}' | grep "$(pwd)" +``` +If found stopped, use `docker start -i `. + +### Starting a build container + +If a `-build` image exists, start a container with the repo mounted: +```bash +docker run -it -v $(pwd):/ovms \ + -e http_proxy=$http_proxy -e https_proxy=$https_proxy -e no_proxy=$no_proxy \ + bash +``` + +If a container from a previous session is available (`docker ps -a`), reuse it: +```bash +docker start -i +# or +docker exec -it bash +``` + +## Bazel Commands (inside build container) + +### Key targets + +| Target | Description | +|--------|-------------| +| `//src:ovms` | Main OVMS server binary | +| `//src:ovms_test` | C++ unit tests (gtest) | +| `//src:ovms_shared` | C API shared library (`libovms_shared.so`) | + +### Build the server +```bash +bazel build //src:ovms +``` + +### Build and run unit tests +```bash +bazel build //src:ovms_test +bazel test --test_summary=detailed --test_output=streamed //src:ovms_test +``` + +### Running specific tests (preferred during development) +```bash +bazel test --test_summary=detailed --test_output=streamed --test_filter="SuiteName.TestName" //src:ovms_test +``` +Always run targeted tests first; run the full suite only after targeted tests pass. + +### Red Hat builds +Pass `--//:distro=redhat` to all Bazel commands: +```bash +bazel build --//:distro=redhat //src:ovms +``` + +### Linux build config +```bash +--config=mp_on_py_on +``` + +### Windows build config +```bash +--config=win_mp_on_py_off +# or with Python: +--config=win_mp_on_py_on +``` + +## Makefile Targets (Docker-based workflow) + +| Target | Description | +|--------|-------------| +| `make docker_build` | Full build: builder image → package → release images | +| `make ovms_builder_image` | Build the `-build` Docker image | +| `make targz_package` | Extract `.tar.gz` release package | +| `make ovms_release_images` | Build CPU and GPU release Docker images | +| `make run_unit_tests` | Run C++ unit tests in the `-build` container | +| `make test_functional` | Run Python functional tests | +| `make style` | All code style checks: spell, clang-format, cpplint, cppclean (see Style Checking section) | + +### Red Hat build via Make +```bash +make docker_build BASE_OS=redhat +``` + +Supported `BASE_OS` values: `ubuntu24`, `ubuntu22`, `redhat` + +## Dockerfile Stages + +Both `Dockerfile.ubuntu` and `Dockerfile.redhat` use multi-stage builds: + +| Stage | Purpose | +|-------|---------| +| `base_build` | System dependencies, Boost, Azure SDK, OpenCV | +| `build` | Bazel + OpenVINO setup, compiles OVMS (the `-build` container) | +| `capi-build` | Builds C API shared library and examples | +| `pkg` | Packages everything into a `.tar.gz` | +| `release` | Minimal runtime image with entrypoint | + +## Style Checking + +**Always run checks individually and sequentially** — never use `make style` (which runs all checks together and wastes time re-running already-passed steps). Fix each step before moving to the next: + +1. **Spelling**: + ```bash + make spell + ``` +2. **clang-format** (formatting): + ```bash + make clang-format-check + ``` +3. **cpplint** (lint rules): + ```bash + make cpplint + ``` +4. **cppclean** (unused includes/code): + ```bash + make cppclean + ``` + +Fix issues from step N before running step N+1 — later steps produce noise if formatting is off. + +## Test Setup + +Before running tests, prepare test models: +```bash +make prepare_models +``` + +If LLM test models need regeneration: +```bash +rm -rf src/test/llm_testing +make prepare_models +``` + +## Windows Builds + +Windows builds use batch files in the repository root: +- `windows_install_build_dependencies.bat` — Install MSVC 2022 Build Tools, etc. +- `windows_build.bat` — Main build script +- `windows_test.bat` — Run tests + +## Test Structure + +- Unit tests are in `src/test/` — gtest-based C++ tests +- Test files: `*_test.cpp` naming convention +- Test utilities: `test_utils.hpp`, `light_test_utils.hpp`, `c_api_test_utils.hpp` +- Test models: `src/test/` subdirectories (`dummy/`, `passthrough/`, `summator/`) +- Specialized: `src/test/llm/`, `src/test/mediapipe/`, `src/test/python/`, `src/test/embeddings/` diff --git a/.github/instructions/cpp-headers.instructions.md b/.github/instructions/cpp-headers.instructions.md new file mode 100644 index 0000000000..f3d258802b --- /dev/null +++ b/.github/instructions/cpp-headers.instructions.md @@ -0,0 +1,25 @@ +--- +applyTo: "**/*.hpp" +--- +# C++ Header Guidelines (OVMS) + +## Include Discipline + +- **Ban umbrella includes**: never use `openvino/openvino.hpp`, `rapidjson/document.h`, or similar in headers. Include only the specific subheader you need. +- **Forward-declare in headers**: if a header only uses pointers or references to a type, write `class Foo;` instead of `#include "foo.hpp"`. Move the full include to the `.cpp`. +- **Self-contained but minimal**: every header must compile on its own (`#pragma once`, own includes), but must not pull in transitive dependencies callers don't need. +- **No header solely for a typedef or enum**: forward-declare the enum (`enum class Foo;`) or use a lightweight `_fwd.hpp` header. +- **Third-party headers**: wrap in a port layer (`src/port/_.hpp`) with warning suppression pragmas. Never include third-party headers directly from multiple OVMS headers. + +## Interface Design + +- **Prefer opaque types / Pimpl**: for complex implementation details, keep impl-only types out of the public header. +- **Narrow interfaces over fat classes**: expose only what consumers need. Use abstract base classes (`ModelInstanceProvider`, `ServableMetadataProvider`, `ServableNameChecker`) returning plain data types. +- **Virtual destructors**: any class with virtual methods must have a virtual destructor. + +## Style + +- No `using namespace std;` or `using namespace ov;` in headers — pollutes every includer's namespace. +- Use `const` and `constexpr` wherever possible. +- Prefer `std::string_view` parameters over `const std::string&` for non-owning read-only access where applicable. +- Member variables: prefer member initializer lists in constructors. diff --git a/.github/instructions/cpp-sources.instructions.md b/.github/instructions/cpp-sources.instructions.md new file mode 100644 index 0000000000..1fbfa81bb9 --- /dev/null +++ b/.github/instructions/cpp-sources.instructions.md @@ -0,0 +1,27 @@ +--- +applyTo: "**/*.cpp" +--- +# C++ Source File Guidelines (OVMS) + +## Include Discipline + +- **Include-what-you-use**: include exactly the headers you need. Do not rely on transitive includes from other headers. +- **Own header first**: the first include in `foo.cpp` should be `"foo.hpp"` — this verifies the header is self-contained. +- **Order**: own header → system/STL → third-party → project headers (alphabetical within each group). + +## Build Target Ownership + +- **Every new `.cpp` must belong to its own `ovms_cc_library` target** in a BUILD file. Do not add sources to monolithic targets like `ovms_lib`. +- If a `.cpp` is only used by tests, use `ovms_test_cc_library` instead. + +## Architecture + +- **Cross-module coupling goes through interfaces**, not concrete classes. If you need `ModelManager` functionality, depend on a narrow interface (`ModelInstanceProvider`, `ServableMetadataProvider`) rather than the full class. +- **Template instantiation isolation**: when a template is parametric on frontend types (TFS/KFS/CAPI protos), put the explicit instantiation in a per-frontend `.cpp` (`dag_tfs.cpp`, `dag_kfs.cpp`) so changing one frontend doesn't recompile others. +- **No dead code**: if a function or variable is unused, remove it. Orphan `.cpp` files not referenced by any BUILD target should be deleted. + +## Performance + +- Avoid copies of large structures (tensors, buffers) — pass by `const&` or move. +- On the hot inference path, avoid `dynamic_cast`, unnecessary allocations, and blocking operations. +- Prefer stack allocation and object reuse over repeated heap allocation in request-handling loops.