📚 API reference Guide made from contributing guide, where the …#1419
📚 API reference Guide made from contributing guide, where the …#1419megnvidia wants to merge 3 commits intonvidia-holoscan:mainfrom
Conversation
…file was aware of the API reference created from the migration guide Signed-off-by: meg miranda <mmiranda@nvidia.com>
Greptile SummaryThis PR adds a new Key findings:
Confidence Score: 4/5Safe to merge once the five broken schema file links are corrected to point to utilities/metadata/. One P1 finding: all schema links in the Schema Locations table are broken and will confuse contributors. The fix is straightforward — update the five paths. All other content is accurate and well-structured. API_REFERENCE.md — the Schema Locations table (lines 26–30) needs corrected paths. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[API_REFERENCE.md] --> B[Metadata Reference]
A --> C[Build System Reference]
A --> D[Directory Structure Reference]
A --> E[Naming Conventions]
A --> F[CLI Command Reference]
A --> G[Testing Reference]
B --> B1["Schema Locations - broken links"]
B --> B2[metadata.json Structure]
B --> B3[Ranking Levels]
B1 -->|"should point to"| B1a["utilities/metadata/*.schema.json"]
C --> C1[Operators CMakeLists]
C --> C2[GXF Extensions CMakeLists]
C --> C3[Applications CMakeLists]
C --> C4[Packages CMakeLists]
F --> F1["utilities/cli/cli_reference.md"]
G --> G1["pytest + CTest"]
Reviews (3): Last reviewed commit: "Merge branch 'main' into mmiranda-AI-API..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
WalkthroughA new comprehensive API reference documentation file is added that details the HoloHub metadata schema structure, path placeholder resolution, CMake build patterns, directory conventions, naming guidelines, CLI commands, and testing approaches across different contribution types. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@API_REFERENCE.md`:
- Around line 323-328: The referenced file utilities/cli/CLI_REFERENCE.md is
missing and linked from the "CLI Command Reference" section and the "See Also"
list; either add the missing CLI_REFERENCE.md file with the CLI command docs or
remove/replace both broken links. Locate the two link occurrences (the link text
or path utilities/cli/CLI_REFERENCE.md in the CLI Command Reference section and
the See Also list) and either create the new markdown file under utilities/cli
with the appropriate CLI content, or update both references to point to an
existing CLI doc (e.g., utilities/cli/README.md or README.md) or remove the
entries so no broken links remain.
- Around line 161-167: The example call to add_holohub_package contains a typo:
the package name is written as my_packager but should match the surrounding
examples and be my_package; update the identifier in the
add_holohub_package(...) invocation to my_package (refer to the
add_holohub_package call and the package identifier symbol my_packager) so the
documentation consistently uses my_package.
- Around line 20-34: Update the requirement sentence that references
metadata.json to include workflows, GXF extensions, and tutorials (in addition
to applications and operators) and expand the "metadata.json Structure
(Overview)" to list the correct root keys for each contribution type: use
"application", "operator", "workflow", "gxf_extension", and "tutorial" (matching
the directories referenced by workflows/metadata.schema.json,
gxf_extensions/metadata.schema.json, and tutorials/metadata.schema.json); ensure
the wording around metadata.json and the schema table consistently documents
which root key to use for each schema type.
- Around line 304-319: Update the example tests to use the defensive import
pattern used in test_pixelator.py: replace the direct import of BaseOperator
(currently `from holoscan.core import Operator, _Operator as BaseOperator`) with
a try/except that first tries `from holoscan.core import BaseOperator` and falls
back to `from holoscan.core import _Operator as BaseOperator`; keep any import
of Operator (for Operator.OperatorType.NATIVE) as-is so test_my_operator_init
still checks operator_type and test_my_operator_invalid_param still raises
ValueError for invalid_param on MyOperatorOp.
- Around line 87-96: Update the documentation table in API_REFERENCE.md to
separate plain workdir placeholder names (used in the "workdir" field) from
command path placeholders (used inside command strings with angle brackets),
i.e., list holohub_bin, holohub_app_bin, holohub_app_source as workdir values
(without angle brackets) and list <holohub_bin>, <holohub_app_bin>,
<holohub_app_source>, <holohub_data_dir> as command path placeholders; also
remove holohub_data_dir from the workdir column and correct the holohub_app_bin
example path from "<holohub_root>/build/myapp/applications/myapp/cpp" to
"<holohub_root>/build/applications/myapp/cpp" so the examples align with how
metadata.json uses "workdir" and "command".
bhashemian
changed the title
|
Closing as it is getting outdated with all the updates to holohub and holoscan ecosystem. please feel free to submit a new PR if needed. Thanks |
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated AI review comments for PR #1419.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new top-level documentation page (API_REFERENCE.md) intended to serve as a quick reference for contributors on metadata schema usage, build integration, directory layout, naming conventions, CLI commands, and testing practices.
Changes:
- Added a consolidated API/reference guide covering
metadata.jsonschemas and common fields. - Documented path placeholder conventions used by the CLI/run tooling.
- Summarized CMake integration patterns, contribution directory structure, CLI commands, and testing guidance.
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated AI review comments for PR #1419.
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated AI review comments for PR #1419.
There was a problem hiding this comment.
- Fix broken links to missing
utilities/cli/CLI_REFERENCE.md
- File/line:
API_REFERENCE.md:256andAPI_REFERENCE.md:326 - Issue: The doc links to
utilities/cli/CLI_REFERENCE.md, but that file does not exist, so the links are broken. - Action: Either add
utilities/cli/CLI_REFERENCE.md(preferred if it’s meant to be the canonical CLI docs) or update/remove both references to point at an existing CLI doc (e.g.,utilities/cli/README.mdif present). - Source: greptile-apps[bot] (comment id
2835568534,2835568564), coderabbitai[bot] (comment id2835573431)
- Clarify
metadata.jsonsupported contribution types + root keys
- File/line:
API_REFERENCE.md:20-34(also overlaps the sentence aroundAPI_REFERENCE.md:34-38) - Issue: The text implies only applications/operators are covered and only
application/operatorroot keys exist, but the repo also has schemas/metadata for other contribution types (e.g., workflows, tutorials, GXF extensions; and possibly benchmark depending on repo policy). - Action: Update the requirement statement and “Root key …” description to explicitly list the supported contribution types and their root keys, or clearly scope the section to only app/operator if that’s intentional.
- Source: coderabbitai[bot] (comment id
2835573420), Copilot (comment id3019244551)
- Document
requirements(notdependencies) and align the example snippet with the schema
- File/line:
API_REFERENCE.md:47andAPI_REFERENCE.md:52-56 - Issue: Documentation calls out a top-level
dependencies, but the schemas and existing metadata use/require arequirementsobject (e.g.,requirements.operators,requirements.gxf_extensions). The example snippet also omitsrequirements, making it fail schema validation when copy/pasted. - Action: Replace/rename the
dependenciesrow torequirementsand add at least a minimalrequirementsobject in the example snippet. - Source: Copilot (comment id
3019244580,3019244637)
- Adjust
tagsdocumentation to match website tooling behavior
- File/line:
API_REFERENCE.md:45 - Issue: Docs say tags are free-form, but doc-site tooling treats the first tag as the category used for filtering/navigation.
- Action: Note that the first tag is the category and should match the approved category list (if that policy is intended), while additional tags can remain free-form.
- Source: Copilot (comment id
3019244614)
- Relax/clarify
versionfield requirements to match schema + existing metadata
- File/line:
API_REFERENCE.md:41-42 - Issue: Docs state strict
major.minor.patch, but existing metadata uses shorter forms like1.0, and the schema regex is looser. - Action: Update wording to “SemVer recommended” (not required) or otherwise align documentation with actual accepted version strings.
- Source: Copilot (comment id
3019244660)
- Fix typo in
add_holohub_packageexample (my_packager→my_package)
- File/line:
API_REFERENCE.md:164(within the block around161-167) - Issue: Example uses
my_packageronce, but the rest of the section usesmy_package, which is confusing when copying the snippet. - Action: Change
add_holohub_package(my_packagertoadd_holohub_package(my_package. - Source: coderabbitai[bot] (comment id
2835573428), Copilot (comment id3019244593)
- Separate
workdirvalues vs command string placeholders; fixholohub_app_binexample path
- File/line:
API_REFERENCE.md:87-96 - Issue: The table conflates:
workdirvalues (plain identifiers likeholohub_bin), and- placeholders embedded in commands (angle-bracketed like
<holohub_data_dir>).
It also incorrectly impliesholohub_data_diris a validworkdirvalue, and theholohub_app_binexample path includes an extra/duplicated segment.
- Action: Split documentation into (a) valid
workdirvalues and (b) valid command placeholders, removeholohub_data_dirfromworkdir, and correct theholohub_app_binexample path to<holohub_root>/build/applications/myapp/cpp. - Source: coderabbitai[bot] (comment id
2835573424)
- Document missing supported CMake dependency options (
DEPENDS OPERATORS/DEPENDS EXTENSIONS, optional markers)
- File/line:
API_REFERENCE.md:113andAPI_REFERENCE.md:127-131 - Issue:
add_holohub_operatordocs only mentionDEPENDS EXTENSIONS, but the helper supportsDEPENDS OPERATORStoo.add_holohub_applicationsupports more thanDEPENDS OPERATORS(e.g.,DEPENDS EXTENSIONSand optional operator dependencies via anOPTIONALmarker). - Action: Expand these sections/examples to list both dependency forms and mention optional dependencies where supported.
- Source: Copilot (comment id
3019244642,3019244648)
- Update test example imports to use the defensive
BaseOperator/_Operatorfallback pattern
- File/line:
API_REFERENCE.md:307(within the example block around304-319) - Issue: Example directly imports
_Operator as BaseOperator, but the referenced real test uses a try/except fallback to support Holoscan versions whereBaseOperatorexists. - Action: Update the example to:
- try importing
BaseOperator, - fall back to
_Operator as BaseOperator, - keep
Operatorimport as needed.
- try importing
- Source: coderabbitai[bot] (comment id
2835573429), Copilot (comment id3019244625)
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated review comments for API_REFERENCE.md
|
/consolidate |
2 similar comments
|
/consolidate |
|
/consolidate |
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated review comments after deduplicating overlapping AI feedback.
|
|
||
| ## CLI Command Reference | ||
|
|
||
| Commands used in the contribution workflow. For full options and behavior, see [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md). |
There was a problem hiding this comment.
Broken documentation link: utilities/cli/CLI_REFERENCE.md is referenced here but the file does not exist in the repository. Either create this documentation file or update the reference to point to an existing CLI documentation file (for example utilities/cli/README.md). The same issue also appears later in this file (around line 326), so both references should be fixed together to avoid broken links.
Source: 2835568534, 2835568564, 2835573431
| Every application and operator must have a `metadata.json` that describes features and dependencies. Contribution-specific schemas define the exact fields. | ||
|
|
||
| ### Schema Locations | ||
|
|
||
| | Type | Schema path | | ||
| |------|-------------| | ||
| | Workflows | [workflows/metadata.schema.json](workflows/metadata.schema.json) | | ||
| | Applications | [applications/metadata.schema.json](applications/metadata.schema.json) | | ||
| | GXF Extensions | [gxf_extensions/metadata.schema.json](gxf_extensions/metadata.schema.json) | | ||
| | Operators | [operators/metadata.schema.json](operators/metadata.schema.json) | | ||
| | Tutorials | [tutorials/metadata.schema.json](tutorials/metadata.schema.json) | | ||
|
|
||
| ### metadata.json Structure (Overview) | ||
|
|
||
| Root key is `application` or `operator` depending on contribution type. Main fields: |
There was a problem hiding this comment.
The metadata documentation appears incomplete/inconsistent with the repository schemas. The text currently implies only application and operator are valid root keys, but the repo defines additional contribution types and schemas (e.g., workflow, tutorial, benchmark, gxf_extension). Please update the description to list all supported root keys and clarify that each contribution type must provide a metadata.json using the appropriate root key.
Source: 2835573420, 3019244551
| | `platforms` | array | for example `["x86_64", "aarch64"]` | | ||
| | `tags` | array | Free-form tags (for example `["Endoscopy", "Video Encoding"]`) | | ||
| | `ranking` | number | Self-assessment level 0–5; see [Ranking levels](#ranking-levels-for-metadatajson) | | ||
| | `dependencies` | object | for example `operators: [{ name, version }]` | |
There was a problem hiding this comment.
The table documents a dependencies top-level field, but the repository schemas and existing metadata.json files use a requirements object instead (e.g., requirements.operators, requirements.gxf_extensions). Please update the documentation to use requirements and describe its structure so the example matches the JSON schemas.
Source: 3019244580
| | `changelog` | object | Keys are versions (for example `"X.X"`), values are short change descriptions | | ||
| | `holoscan_sdk` | object | `minimum_required_version`, `tested_versions` (array) | | ||
| | `platforms` | array | for example `["x86_64", "aarch64"]` | | ||
| | `tags` | array | Free-form tags (for example `["Endoscopy", "Video Encoding"]`) | |
There was a problem hiding this comment.
The tags description says tags are free-form, but repository tooling treats the first tag as the category used for documentation filtering/navigation. Consider documenting that the first tag must correspond to an approved category (e.g., from .github/copilot-instructions.md) while additional tags may remain free-form.
Source: 3019244614
| | `name` | string | Explicit name of the application or operator | | ||
| | `authors` | array | Objects with `name` and `affiliation` | | ||
| | `language` | string | `C++`, `Python`, or `GXF`; use one directory per language if multiple | | ||
| | `version` | string | Semantic version: `major.minor.patch` | |
There was a problem hiding this comment.
The version field is documented as strict major.minor.patch, but many existing metadata files in the repository use shorter forms such as 1.0, and the JSON schema appears less strict than full SemVer. Consider updating the description to clarify that major.minor.patch is recommended but shorter forms like 1.0 are accepted.
Source: 3019244660
| In `metadata.json`, the `workdir` for the run command can be one of these. They are resolved at build/run time: | ||
|
|
||
| | Placeholder | Meaning | | ||
| |-------------|---------| | ||
| | `holohub_app_bin` | Directory containing the built application binary (for example `<holohub_root>/build/myapp/applications/myapp/cpp`) | | ||
| | `holohub_app_source` | Application source directory (for example `<holohub_root>/applications/myapp/cpp/`) | | ||
| | `holohub_bin` | Root build directory (for example `<holohub_root>/build/`) | | ||
| | `holohub_data_dir` | Data directory (for example `<holohub_root>/data/`) | | ||
|
|
||
| Use `<holohub_data_dir>` in the run `command` for data paths (for example `--data <holohub_data_dir>/mydata`). |
There was a problem hiding this comment.
The placeholder table mixes two different concepts: (1) values used for the workdir field and (2) placeholders embedded in command strings (with angle brackets). In actual metadata.json usage, workdir uses plain names (e.g., "workdir": "holohub_app_bin") while command strings use placeholders like <holohub_app_bin> or <holohub_data_dir>. Consider splitting the documentation into two sections (valid workdir values vs. command placeholders) and removing holohub_data_dir from the workdir list. Also correct the holohub_app_bin example path to <holohub_root>/build/applications/myapp/cpp.
Source: 2835573424
| ``` | ||
|
|
||
| - `my_operator`: operator target name (matches directory under `operators/`). | ||
| - `DEPENDS EXTENSIONS` (optional): list of GXF extension targets this operator wraps; build system will build them first. |
There was a problem hiding this comment.
add_holohub_operator supports dependency declarations for both extensions and operators (DEPENDS EXTENSIONS and DEPENDS OPERATORS), but this section only documents DEPENDS EXTENSIONS. Consider updating the description to include both supported dependency types so contributors are aware of the full capability.
Source: 3019244642
| ```cmake | ||
| add_holohub_application(my_application DEPENDS OPERATORS my_operator1 my_operator2) | ||
| ``` | ||
|
|
||
| - `DEPENDS OPERATORS` (optional): operator targets required by this application. |
There was a problem hiding this comment.
The documentation for add_holohub_application currently mentions only DEPENDS OPERATORS, but the build helpers also support DEPENDS EXTENSIONS and optional dependencies (via the OPTIONAL marker). Consider expanding the description or example so contributors know these options are available.
Source: 3019244648
| **Package registration** (in `./pkg/CMakeLists.txt`): | ||
|
|
||
| ```cmake | ||
| add_holohub_package(my_packager |
There was a problem hiding this comment.
Typo in the example: add_holohub_package(my_packager ...) is inconsistent with the surrounding examples that use my_package. This likely should be add_holohub_package(my_package ...) to avoid confusion when users copy the example.
Source: 2835573428, 3019244593
| ```python | ||
| import pytest | ||
| from .my_operator import MyOperatorOp | ||
| from holoscan.core import Operator, _Operator as BaseOperator |
There was a problem hiding this comment.
The example test imports _Operator directly (from holoscan.core import Operator, _Operator as BaseOperator), but the reference implementation (operators/deidentification/pixelator/test_pixelator.py) uses a defensive import to support multiple Holoscan versions. Consider updating the example to the compatibility pattern:
try:
from holoscan.core import Operator, BaseOperator
except ImportError:
from holoscan.core import Operator, _Operator as BaseOperator
This avoids giving contributors a brittle import pattern.
Source: 2835573429, 3019244625
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated review comments from AI analyses.
|
|
||
| ## CLI Command Reference | ||
|
|
||
| Commands used in the contribution workflow. For full options and behavior, see [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md). |
There was a problem hiding this comment.
The documentation references utilities/cli/CLI_REFERENCE.md, but this file does not appear to exist in the repository. Please either add the missing file or update the link to point to the correct existing documentation. This same broken reference also appears later in the file (e.g., line 326) and should be fixed consistently.
Source: 2835568534, 2835568564
| **Package registration** (in `./pkg/CMakeLists.txt`): | ||
|
|
||
| ```cmake | ||
| add_holohub_package(my_packager | ||
| APPLICATIONS my_app1 | ||
| OPERATORS my_op1 my_op2) | ||
| ``` |
There was a problem hiding this comment.
There appears to be a typo in the example add_holohub_package call. The package name is written as my_packager, while the surrounding documentation consistently uses my_package. Consider updating the example for consistency:
add_holohub_package(my_package APPLICATIONS my_app1 OPERATORS my_op1 my_op2)
Source: 2835573428
| ```python | ||
| import pytest | ||
| from .my_operator import MyOperatorOp | ||
| from holoscan.core import Operator, _Operator as BaseOperator | ||
|
|
||
| def test_my_operator_init(fragment): | ||
| op = MyOperatorOp(fragment=fragment, name="myoperator_op", tensor_name="image") | ||
| assert isinstance(op, BaseOperator) | ||
| assert op.operator_type == Operator.OperatorType.NATIVE | ||
|
|
||
| def test_my_operator_invalid_param(fragment): | ||
| with pytest.raises(ValueError): | ||
| MyOperatorOp(fragment=fragment, tensor_name="image", invalid_param=-1) | ||
| ``` | ||
|
|
||
| Reference implementations: for example `operators/deidentification/pixelator/test_pixelator.py` and `conftest.py` for fixtures. |
There was a problem hiding this comment.
The example test code imports BaseOperator differently from the pattern used in existing tests such as operators/deidentification/pixelator/test_pixelator.py. To maintain compatibility with different holoscan.core versions, consider using the defensive import pattern used in the repository:
try:
from holoscan.core import BaseOperator
except ImportError:
from holoscan.core import _Operator as BaseOperator
Keeping the example aligned with the repository’s test patterns will make the documentation more robust to API differences across versions.
Source: 2835573429
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated review comments from AI analyses.
|
|
||
| ## CLI Command Reference | ||
|
|
||
| Commands used in the contribution workflow. For full options and behavior, see [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md). |
There was a problem hiding this comment.
The documentation references utilities/cli/CLI_REFERENCE.md, but this file does not appear to exist in the repository. Please either add the missing file or update the link to point to the correct existing documentation. This same broken reference also appears later in the file (e.g., line 326) and should be fixed consistently.
Source: 2835568534, 2835568564
| **Package registration** (in `./pkg/CMakeLists.txt`): | ||
|
|
||
| ```cmake | ||
| add_holohub_package(my_packager | ||
| APPLICATIONS my_app1 | ||
| OPERATORS my_op1 my_op2) | ||
| ``` |
There was a problem hiding this comment.
There appears to be a typo in the example add_holohub_package call. The package name is written as my_packager, while the surrounding documentation consistently uses my_package. Consider updating the example for consistency:
add_holohub_package(my_package APPLICATIONS my_app1 OPERATORS my_op1 my_op2)
Source: 2835573428
| ```python | ||
| import pytest | ||
| from .my_operator import MyOperatorOp | ||
| from holoscan.core import Operator, _Operator as BaseOperator | ||
|
|
||
| def test_my_operator_init(fragment): | ||
| op = MyOperatorOp(fragment=fragment, name="myoperator_op", tensor_name="image") | ||
| assert isinstance(op, BaseOperator) | ||
| assert op.operator_type == Operator.OperatorType.NATIVE | ||
|
|
||
| def test_my_operator_invalid_param(fragment): | ||
| with pytest.raises(ValueError): | ||
| MyOperatorOp(fragment=fragment, tensor_name="image", invalid_param=-1) | ||
| ``` | ||
|
|
||
| Reference implementations: for example `operators/deidentification/pixelator/test_pixelator.py` and `conftest.py` for fixtures. |
There was a problem hiding this comment.
The example test code imports BaseOperator differently from the pattern used in existing tests such as operators/deidentification/pixelator/test_pixelator.py. To maintain compatibility with different holoscan.core versions, consider using the defensive import pattern used in the repository:
try:
from holoscan.core import BaseOperator
except ImportError:
from holoscan.core import _Operator as BaseOperator
Keeping the example aligned with the repository’s test patterns will make the documentation more robust to API differences across versions.
Source: 2835573429
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated review comments from AI analyses.
|
|
||
| ## CLI Command Reference | ||
|
|
||
| Commands used in the contribution workflow. For full options and behavior, see [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md). |
There was a problem hiding this comment.
The documentation references utilities/cli/CLI_REFERENCE.md, but this file does not appear to exist in the repository. Please either add the missing file or update the link to point to the correct existing documentation. This same broken reference also appears later in the file (e.g., line 326) and should be fixed consistently.
Source: 2835568534, 2835568564
| **Package registration** (in `./pkg/CMakeLists.txt`): | ||
|
|
||
| ```cmake | ||
| add_holohub_package(my_packager | ||
| APPLICATIONS my_app1 | ||
| OPERATORS my_op1 my_op2) | ||
| ``` |
There was a problem hiding this comment.
There appears to be a typo in the example add_holohub_package call. The package name is written as my_packager, while the surrounding documentation consistently uses my_package. Consider updating the example for consistency:
add_holohub_package(my_package APPLICATIONS my_app1 OPERATORS my_op1 my_op2)
Source: 2835573428
| ```python | ||
| import pytest | ||
| from .my_operator import MyOperatorOp | ||
| from holoscan.core import Operator, _Operator as BaseOperator | ||
|
|
||
| def test_my_operator_init(fragment): | ||
| op = MyOperatorOp(fragment=fragment, name="myoperator_op", tensor_name="image") | ||
| assert isinstance(op, BaseOperator) | ||
| assert op.operator_type == Operator.OperatorType.NATIVE | ||
|
|
||
| def test_my_operator_invalid_param(fragment): | ||
| with pytest.raises(ValueError): | ||
| MyOperatorOp(fragment=fragment, tensor_name="image", invalid_param=-1) | ||
| ``` | ||
|
|
||
| Reference implementations: for example `operators/deidentification/pixelator/test_pixelator.py` and `conftest.py` for fixtures. |
There was a problem hiding this comment.
The example test code imports BaseOperator differently from the pattern used in existing tests such as operators/deidentification/pixelator/test_pixelator.py. To maintain compatibility with different holoscan.core versions, consider using the defensive import pattern used in the repository:
try:
from holoscan.core import BaseOperator
except ImportError:
from holoscan.core import _Operator as BaseOperator
Keeping the example aligned with the repository’s test patterns will make the documentation more robust to API differences across versions.
Source: 2835573429
|
|
||
| ## CLI Command Reference | ||
|
|
||
| Commands used in the contribution workflow. For full options and behavior, see [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md). |
There was a problem hiding this comment.
The documentation references utilities/cli/CLI_REFERENCE.md, but this file does not appear to exist in the repository. Please verify the path. If the CLI reference is intended, consider adding the file or updating the link to point to the correct existing documentation location.
Source: 2835568534
| Every application and operator must have a `metadata.json` that describes features and dependencies. Contribution-specific schemas define the exact fields. | ||
|
|
||
| ### Schema Locations | ||
|
|
||
| | Type | Schema path | | ||
| |------|-------------| | ||
| | Workflows | [workflows/metadata.schema.json](workflows/metadata.schema.json) | | ||
| | Applications | [applications/metadata.schema.json](applications/metadata.schema.json) | | ||
| | GXF Extensions | [gxf_extensions/metadata.schema.json](gxf_extensions/metadata.schema.json) | | ||
| | Operators | [operators/metadata.schema.json](operators/metadata.schema.json) | | ||
| | Tutorials | [tutorials/metadata.schema.json](tutorials/metadata.schema.json) | | ||
|
|
||
| ### metadata.json Structure (Overview) | ||
|
|
||
| Root key is `application` or `operator` depending on contribution type. Main fields: |
There was a problem hiding this comment.
The description of metadata.json requirements appears incomplete. The text currently mentions only applications and operators and lists root keys application and operator, but the repository also contains other contribution types (e.g., workflows, GXF extensions, and tutorials). Consider updating this section to clarify that each contribution type requires a metadata.json, and document the corresponding root keys (e.g., application, operator, workflow, gxf_extension, tutorial) so contributors for all types know which schema and root key to use.
Source: 2835573420
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Consolidated review comments for the pull request.
| ## See Also | ||
|
|
||
| - [CONTRIBUTING.md](CONTRIBUTING.md) — Contribution workflow, checklists, and guidelines | ||
| - [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md) — Full HoloHub CLI reference | ||
| - [README.md](README.md) — Project overview and glossary | ||
| - [doc/developer.md](doc/developer.md) — Advanced developer guide |
There was a problem hiding this comment.
Broken link reference to utilities/cli/CLI_REFERENCE.md. The file does not exist in the repository but is referenced in the documentation (including this section and another earlier occurrence around line 256). This will lead to dead documentation links. Either add the missing utilities/cli/CLI_REFERENCE.md file with the intended CLI documentation, or update/remove the references to point to an existing document (for example utilities/cli/README.md if that is the correct location).
Source: 2835573431
bhashemian
requested review from
a team and
Copilot
and removed request for
a team
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Could you please address the consolidated comments below? Thanks!
| Every application and operator must have a `metadata.json` that describes features and dependencies. Contribution-specific schemas define the exact fields. | ||
|
|
||
| ### Schema Locations | ||
|
|
||
| | Type | Schema path | | ||
| |------|-------------| | ||
| | Workflows | [workflows/metadata.schema.json](workflows/metadata.schema.json) | | ||
| | Applications | [applications/metadata.schema.json](applications/metadata.schema.json) | | ||
| | GXF Extensions | [gxf_extensions/metadata.schema.json](gxf_extensions/metadata.schema.json) | | ||
| | Operators | [operators/metadata.schema.json](operators/metadata.schema.json) | | ||
| | Tutorials | [tutorials/metadata.schema.json](tutorials/metadata.schema.json) | | ||
|
|
||
| ### metadata.json Structure (Overview) | ||
|
|
||
| Root key is `application` or `operator` depending on contribution type. Main fields: |
There was a problem hiding this comment.
The documentation of metadata.json is incomplete and may mislead contributors. The text states that the root key is application or operator, but the repository supports additional contribution types. Please update the description to include all supported root keys and ensure the requirement statement reflects all contribution categories (applications, operators, workflows, GXF extensions, tutorials, and benchmarks if applicable). For example, clarify that the root key must be one of: application, operator, workflow, tutorial, benchmark, or gxf_extension, and ensure the surrounding explanation consistently reflects these contribution types.
Source: 2835573420, 3029922104
| | Workflows | [workflows/metadata.schema.json](workflows/metadata.schema.json) | | ||
| | Applications | [applications/metadata.schema.json](applications/metadata.schema.json) | | ||
| | GXF Extensions | [gxf_extensions/metadata.schema.json](gxf_extensions/metadata.schema.json) | | ||
| | Operators | [operators/metadata.schema.json](operators/metadata.schema.json) | | ||
| | Tutorials | [tutorials/metadata.schema.json](tutorials/metadata.schema.json) | |
There was a problem hiding this comment.
The schema links in this table appear to point to paths that do not exist. The repository’s metadata schemas are located under utilities/metadata/ (e.g., utilities/metadata/application.schema.json, workflow.schema.json, operator.schema.json, tutorial.schema.json, gxf_extension.schema.json, benchmark.schema.json). Please update the table links to reference the correct paths so contributors can navigate to the actual schema files.
Source: 3029922061
| | `name` | string | Explicit name of the application or operator | | ||
| | `authors` | array | Objects with `name` and `affiliation` | | ||
| | `language` | string | `C++`, `Python`, or `GXF`; use one directory per language if multiple | | ||
| | `version` | string | Semantic version: `major.minor.patch` | |
There was a problem hiding this comment.
The example describes language as a string, but existing metadata.json files and the schema allow it to be either a string or an array (e.g., ["C++", "Python"]). Please clarify in the documentation that language may be a single string or a list when multiple languages are used.
Source: 3029922129
| **Package registration** (in `./pkg/CMakeLists.txt`): | ||
|
|
||
| ```cmake | ||
| add_holohub_package(my_packager | ||
| APPLICATIONS my_app1 | ||
| OPERATORS my_op1 my_op2) | ||
| ``` |
There was a problem hiding this comment.
There appears to be a typo in the add_holohub_package example: the package name is written as my_packager, while surrounding examples and documentation consistently use my_package. Please update the example so the identifier is consistent.
Source: 2835573428
| ├── metadata.json # Required; operator schema | ||
| ├── README.md # Required | ||
| ├── your_operator_name.py | .cpp | .hpp | ||
| ├── test_your_operator_name.py # Required for Python operators |
There was a problem hiding this comment.
The directory tree marks test_<operator>.py as "Required for Python operators", but project guidance elsewhere describes Python operator unit tests as strongly encouraged rather than strictly required. Please align the wording here with the actual policy to avoid confusing contributors (e.g., mark it as "Strongly encouraged for Python operators").
Source: 3029922139
| ## CLI Command Reference | ||
|
|
||
| Commands used in the contribution workflow. For full options and behavior, see [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md). | ||
|
|
There was a problem hiding this comment.
The link to the CLI reference uses utilities/cli/CLI_REFERENCE.md, but the file in the repository is utilities/cli/cli_reference.md (lowercase). On case-sensitive filesystems this will result in a broken link. Please update the link target to the correct filename.
Source: 2835568534, 3029922119
| ## See Also | ||
|
|
||
| - [CONTRIBUTING.md](CONTRIBUTING.md) — Contribution workflow, checklists, and guidelines | ||
| - [utilities/cli/CLI_REFERENCE.md](utilities/cli/CLI_REFERENCE.md) — Full HoloHub CLI reference |
There was a problem hiding this comment.
This link again references utilities/cli/CLI_REFERENCE.md, but the actual file is utilities/cli/cli_reference.md (lowercase). Please update the link to the correct filename to prevent 404s on case-sensitive filesystems.
Source: 2835568564, 3029922151
| ```json | ||
| "application": { | ||
| "name": "my_application", | ||
| "authors": [{ "name": "Your Name", "affiliation": "Your Organization" }], | ||
| "language": "C++", | ||
| "version": "1.0.0", | ||
| "holoscan_sdk": { | ||
| "minimum_required_version": "0.6.0", | ||
| "tested_versions": ["0.6.0"] | ||
| }, | ||
| "platforms": ["x86_64", "aarch64"], | ||
| "tags": ["Video"], | ||
| "ranking": 2, | ||
| "run": { | ||
| "command": "./myapplication --data <holohub_data_dir>/mydata", | ||
| "workdir": "holohub_app_bin" | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The example metadata.json snippet is missing fields that are required by the current metadata schemas/validator (notably changelog and requirements). As written, contributors who copy this snippet will fail utilities/metadata/metadata_validator.py validation; please update the snippet to include all required keys for an application/operator.
FOR TESTING PURPOSES ONLY!
PLEASE DO NOT REVIEW NOR MERGE THIS PR! THANKS!
…file was aware of the API reference created from the migration guide
I can try a version next week that would be without the API reference created from the migration guide.
With no main Sphinx guide there are many ways to manage this content. There is no best. There are just multiple very good options.
Summary by CodeRabbit
Release Notes