fix: ensure workflows set a FunctionGroups instance_name#815
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
WalkthroughAdds a public mutator and read-only property to FunctionGroup to change and read its instance name at runtime; WorkflowBuilder now sets that instance name on the built FunctionGroup before returning the configured result. Tests updated to use the new instance-name-based function identifiers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant WB as WorkflowBuilder
participant Builder as GroupBuilder
participant FG as FunctionGroup
Caller->>WB: _build_function_group(name, config, ...)
WB->>Builder: build(...)
Builder-->>WB: FunctionGroup instance (FG)
WB->>FG: set_instance_name(name)
note right of FG #e6f7ff: Updates internal _instance_name\naffects future identifier generation
WB-->>Caller: ConfiguredFunctionGroup(config, instance=FG)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/nat/builder/test_builder.py (1)
1156-1163: Prefer public APIs over private registry access in testsReaching into builder._functions couples tests to internals. Use group introspection and get_function instead.
- assert "math_group.add" in builder._functions - assert "math_group.multiply" in builder._functions - # Test that non-included functions are not accessible - assert "math_group.subtract" not in builder._functions - # Test that no functions were included from empty group - empty_group_functions = [k for k in builder._functions.keys() if k.startswith("empty_group.")] + group = builder.get_function_group("math_group") + assert set(group.get_accessible_functions().keys()) == {"math_group.add", "math_group.multiply"} + # Non-included function should not be globally accessible + with pytest.raises(ValueError): + builder.get_function("math_group.subtract") + # Verify empty group exposes nothing + empty_group = builder.get_function_group("empty_group") + assert empty_group.get_accessible_functions() == {}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/nat/builder/test_builder.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Unit tests must live under tests/ and use configured markers (e2e, integration, etc.)
Files:
tests/nat/builder/test_builder.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/builder/test_builder.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Follow PEP 8/20 style; format with yapf (column_limit=120) and use 4-space indentation; end files with a single newline
Run ruff (ruff check --fix) per pyproject.toml; fix warnings unless explicitly ignored; ruff is linter-only
Use snake_case for functions/variables, PascalCase for classes, and UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: preserve stack traces and avoid duplicate logging
When re-raising exceptions, use bareraiseand log with logger.error(), not logger.exception()
When catching and not re-raising, log with logger.exception() to capture stack trace
Validate and sanitize all user input; prefer httpx with SSL verification and follow OWASP Top‑10
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile/mprof; cache with functools.lru_cache or external cache; leverage NumPy vectorization when beneficial
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
Files:
tests/nat/builder/test_builder.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/tests/**/*.py: Test functions must use the test_ prefix and snake_case
Extract repeated test code into pytest fixtures; fixtures should set name=... in @pytest.fixture and functions named with fixture_ prefix
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration
Use pytest with pytest-asyncio for async code; mock external services with pytest_httpserver or unittest.mock
Files:
tests/nat/builder/test_builder.py
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
tests/nat/builder/test_builder.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never hard‑code version numbers in code or docs; versions are derived by setuptools‑scm
Files:
tests/nat/builder/test_builder.py
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
tests/nat/builder/test_builder.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
tests/nat/builder/test_builder.py
🧬 Code graph analysis (1)
tests/nat/builder/test_builder.py (3)
src/nat/builder/workflow_builder.py (4)
get_function(462-468)get_function(1130-1136)get_function_group(471-477)get_function_group(1139-1145)src/nat/builder/builder.py (2)
get_function(75-76)get_function_group(79-80)src/nat/builder/function.py (1)
get_accessible_functions(470-506)
🔇 Additional comments (6)
tests/nat/builder/test_builder.py (6)
1032-1036: Exclude-only group assertions are correctNo globals exposed; availability verified via group-accessible functions using instance prefix.
Also applies to: 1044-1044
1075-1077: All-includes group: correct global exposure via instance prefixLookups using "all_includes_group.*" are accurate.
1099-1103: All-excludes group: correct non-exposure globallyExpectations for ValueError on global lookups are right.
1229-1231: Excluded set uses instance-name prefixes — goodAsserting "excludes_group.*" in excluded list matches the new naming.
1312-1316: Execution via instance-name retrieval worksRetrieval and ainvoke of "math_group.add"/"multiply" validates the end-to-end behavior.
1011-1012: Instance-name lookups updated correctlySwitched tests to "includes_group.". Ran the suggested regex search for the old group-type prefixes followed by '.' — no matches found; tests/nat/builder/test_builder.py (lines 1011–1012 and 1019) reference includes_group..
|
/merge |
2 participants
Description
Functions have their corresponding "name" be whatever what specified in the workflow.
Function Groups should do the same.
This PR addresses an overlooked feature parity with Functions by automatically setting the
instance_name(e.g. what is presented to the tools / user) based on the workflow component's name.Example
Before:
Would expose as:
my_fn_group.fooandmy_fn_group.barAfter:
Will expose as:
my-group.fooandmy-group.barBy Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Tests