feat(core): expose read-only views of function group filters - #2147
Conversation
Signed-off-by: David Hyde <DABH@users.noreply.github.com>
Walkthrough
ChangesFunctionGroup filter accessors
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
I get your point, but at the same time, we intentionally don't want to expose |
|
Fair - agreed the filter callables themselves shouldn't be public, and manual inspection works for tests. The narrower need I was reaching for is that at runtime, a plugin that validates configuration at startup wants to know whether a group's membership is dynamic (filtered), because dynamic membership changes what can be verified up front. It never needs the filters themselves though. Would a minimal |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nvidia_nat_core/tests/nat/builder/test_function_group.py (1)
318-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest direct assignment rejection for
filter_fn.This test verifies getter values and
set_filter_fn, but it does not verify the read-only property contract. Add an assertion that direct assignment raisesAttributeError.Proposed test
group.set_filter_fn(group_filter) assert group.filter_fn is group_filter + + with pytest.raises(AttributeError): + setattr(group, "filter_fn", group_filter)As per path instructions, tests must be comprehensive and validate the functionality. The coding guidelines also require tests when introducing changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_core/tests/nat/builder/test_function_group.py` around lines 318 - 335, Extend test_function_group_filter_fn_property to verify the read-only contract by asserting that direct assignment to group.filter_fn raises AttributeError, while preserving the existing getter and set_filter_fn assertions.Sources: Coding guidelines, Path instructions
packages/nvidia_nat_core/src/nat/builder/function.py (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep filter callbacks private.
These properties expose callback objects and the per-function callback map as public API. A read-only wrapper prevents map mutation, but it does not keep the callback implementation private or limit future compatibility commitments.
If the runtime only needs to detect dynamic membership, expose a boolean instead:
Proposed API narrowing
- from collections.abc import Mapping from collections.abc import Sequence - from types import MappingProxyType - `@property` - def filter_fn(self) -> Callable[[Sequence[str]], Awaitable[Sequence[str]]] | None: - ... - return self._filter_fn - - `@property` - def per_function_filter_fns(self) -> Mapping[str, Callable[[str], Awaitable[bool]]]: - ... - return MappingProxyType(self._per_function_filter_fn) + `@property` + def dynamic_membership(self) -> bool: + """Returns whether function-group membership can change dynamically.""" + return self._filter_fn is not None or bool(self._per_function_filter_fn)Update the new tests to validate
dynamic_membershipinstead. The PR objective states that runtime validation needs only dynamic-membership state. As per path instructions, changes in core functionality should prioritize backward compatibility.Also applies to: 821-836
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_core/src/nat/builder/function.py` around lines 24 - 26, Narrow the function builder API by removing public callback-object and per-function callback-map exposure, replacing them with a boolean dynamic_membership state used by runtime validation. Update the related tests to assert dynamic_membership rather than inspecting callbacks or callback mappings, while preserving existing behavior and compatibility for non-dynamic functions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/nvidia_nat_core/src/nat/builder/function.py`:
- Around line 24-26: Narrow the function builder API by removing public
callback-object and per-function callback-map exposure, replacing them with a
boolean dynamic_membership state used by runtime validation. Update the related
tests to assert dynamic_membership rather than inspecting callbacks or callback
mappings, while preserving existing behavior and compatibility for non-dynamic
functions.
In `@packages/nvidia_nat_core/tests/nat/builder/test_function_group.py`:
- Around line 318-335: Extend test_function_group_filter_fn_property to verify
the read-only contract by asserting that direct assignment to group.filter_fn
raises AttributeError, while preserving the existing getter and set_filter_fn
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20c18d55-3d25-4185-9c3a-033ac1745c97
📒 Files selected for processing (2)
packages/nvidia_nat_core/src/nat/builder/function.pypackages/nvidia_nat_core/tests/nat/builder/test_function_group.py
2 participants
Description
FunctionGroupis part of the public plugin-authoring contract: third-party plugin packages import it from the stablenat.plugin_apifacade (perdocs/source/extend/third-party-plugins.md, symbols exported fromnat.plugin_apiare the public contract for external plugin packages) and build provider tool groups throughregister_function_group. A group's composition can be shaped by two kinds of filter callbacks: a group-levelfilter_fn(constructor argument, orset_filter_fn) and per-function filter callbacks (add_function(..., filter_fn=...), orset_per_function_filter_fn). Today both are write-only from the outside: a plugin package that wants to validate, document, or instrument how a group is composed (for example, asserting in its own tests that the expected filters were wired up, or logging which functions are dynamically gated) has to reach into the private_filter_fnand_per_function_filter_fnattributes, which is exactly the kind of implementation-module reliance the third-party plugin guide tells authors to avoid.This change adds two minimal read-only properties to
FunctionGroup, mirroring the existinginstance_nameandmiddlewareproperties:filter_fnreturns the configured group-level filter callback, orNonewhen no group-level filter has been set.per_function_filter_fnsreturns an immutabletypes.MappingProxyTypeview of the per-function filter callbacks keyed by function name. The view rejects mutation withTypeErrorand stays live: filter callbacks registered later (throughadd_functionorset_per_function_filter_fn) appear in a previously obtained view.The change is purely additive: no existing behavior changes, and no new module-level symbols are added to
nat.plugin_api(the properties ride along on the already-exportedFunctionGroupclass, so the pinned export test and the plugin API surface documentation are unaffected).No tracking issue exists for this yet; happy to file one if the team prefers.
Testing
uv run pytest packages/nvidia_nat_core/tests/nat/builder/test_function_group.py— 28 passed (includes two new tests covering groups constructed with and without filters, filters set after construction, read-only enforcement on the mapping view, and the view reflecting later additions).uv run pytest packages/nvidia_nat_core/tests/nat/builder— 319 passed.uv run pre-commit run yapf --files packages/nvidia_nat_core/src/nat/builder/function.py packages/nvidia_nat_core/tests/nat/builder/test_function_group.py— passed.uv run pre-commit run ruff-check --files packages/nvidia_nat_core/src/nat/builder/function.py packages/nvidia_nat_core/tests/nat/builder/test_function_group.py— passed.uv run python ci/scripts/copyright.py --verify-apache-v2— passed.By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Bug Fixes