-
Notifications
You must be signed in to change notification settings - Fork 73
fix: @Bartok9 contributions #824
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: main
Are you sure you want to change the base?
Changes from all commits
8968e2b
fbb88c2
f672311
3501232
4698adc
72ed265
042cc25
81932de
460367f
ca14da2
a51057b
5e0a2bc
e3c0ffe
81f5a89
082be4b
e1745a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |||||||||||||||||||||||||||||
| from langchain_core.language_models import BaseChatModel | ||||||||||||||||||||||||||||||
| from langchain_core.messages import BaseMessage, HumanMessage | ||||||||||||||||||||||||||||||
| from langchain_core.tools import BaseTool | ||||||||||||||||||||||||||||||
| from pydantic import BaseModel, ConfigDict, Field | ||||||||||||||||||||||||||||||
| from pydantic import BaseModel, ConfigDict, Field, field_validator | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| from rai.agents.langchain.agent import LangChainAgent, newMessageBehaviorType | ||||||||||||||||||||||||||||||
| from rai.agents.langchain.core import ReActAgentState, create_state_based_runnable | ||||||||||||||||||||||||||||||
|
|
@@ -42,6 +42,20 @@ class StateBasedConfig(BaseModel): | |||||||||||||||||||||||||||||
| arbitrary_types_allowed=True, | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @field_validator("time_interval") | ||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||
| def _time_interval_positive(cls, v: float) -> float: | ||||||||||||||||||||||||||||||
| if v <= 0: | ||||||||||||||||||||||||||||||
| raise ValueError(f"time_interval must be positive, got {v!r}") | ||||||||||||||||||||||||||||||
| return v | ||||||||||||||||||||||||||||||
|
Comment on lines
+45
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Reject non-finite
Proposed validation+import math
+
`@field_validator`("time_interval")
`@classmethod`
def _time_interval_positive(cls, v: float) -> float:
- if v <= 0:
+ if not math.isfinite(v) or v <= 0:
raise ValueError(f"time_interval must be positive, got {v!r}")📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @field_validator("max_workers") | ||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||
| def _max_workers_positive(cls, v: int) -> int: | ||||||||||||||||||||||||||||||
| if v <= 0: | ||||||||||||||||||||||||||||||
| raise ValueError(f"max_workers must be positive, got {v!r}") | ||||||||||||||||||||||||||||||
| return v | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| class BaseStateBasedAgent(LangChainAgent, ABC): | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,8 @@ class BaseAggregator(ABC, Generic[T]): | |
|
|
||
| def __init__(self, max_size: int | None = None) -> None: | ||
| super().__init__() | ||
| if max_size is not None and max_size <= 0: | ||
| raise ValueError(f"max_size must be positive or None, got {max_size}") | ||
|
Comment on lines
+31
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Use one consistent integer-size validation contract. Both constructors accept positive floats and booleans as buffer sizes, causing inaccurate queue capacities. Apply the same non-boolean positive-integer validation in both locations, while preserving
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| self._buffer: Deque[T] = deque() | ||
| self.max_size = max_size | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,8 +152,17 @@ def create_action_server( | |
| The handle for the created action server | ||
|
|
||
| Raises: | ||
| ValueError: If the action server cannot be created | ||
| TypeError: If result_timeout is not a positive number (bool rejected). | ||
| ValueError: If the action server cannot be created or result_timeout <= 0. | ||
| """ | ||
| if isinstance(result_timeout, bool) or not isinstance( | ||
| result_timeout, (int, float) | ||
| ): | ||
| raise TypeError( | ||
| f"result_timeout must be a positive number, got {type(result_timeout).__name__}" | ||
| ) | ||
| if result_timeout <= 0: | ||
| raise ValueError(f"result_timeout must be positive, got {result_timeout!r}") | ||
|
Comment on lines
+155
to
+165
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Keep timeout validation consistent across both APIs.
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| handle = self._generate_handle() | ||
| action_ros_type = import_message_from_str(action_type) | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,8 +30,8 @@ def get_missing_entities( | |
| callable_get_entities: Callable[[], List[str]], | ||
| requested_entities: List[str], | ||
| ) -> set[str]: | ||
| requested_set = set(requested_entities) | ||
| available_set = set(callable_get_entities()) | ||
| requested_set = {ensure_slash(name) for name in requested_entities} | ||
| available_set = {ensure_slash(name) for name in callable_get_entities()} | ||
|
Comment on lines
+33
to
+34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Normalize the timeout-reporting path too. Polling uses normalized sets, but the final timeout calculation still compares normalized Proposed fix- available_entities = get_entities()
-
- missing = set(requested) - set(available_entities)
+ missing = get_missing_entities(get_entities, requested)🤖 Prompt for AI Agents |
||
| return requested_set - available_set | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,18 +26,18 @@ def store_artifacts( | |
| tool_call_id: str, artifacts: List[Any], db_path="artifact_database.pkl" | ||
| ): | ||
| # TODO(boczekbartek): refactor | ||
| db_path = Path(db_path) | ||
| if not db_path.is_file(): | ||
| artifact_database = {} | ||
| with open("artifact_database.pkl", "wb") as file: | ||
| path = Path(db_path) | ||
| if not path.is_file(): | ||
| artifact_database: dict = {} | ||
| with path.open("wb") as file: | ||
| pickle.dump(artifact_database, file) | ||
|
Comment on lines
+29
to
33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Fix the missing-parent contract in both implementation and test.
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| with open("artifact_database.pkl", "rb") as file: | ||
| with path.open("rb") as file: | ||
| artifact_database = pickle.load(file) | ||
| if tool_call_id not in artifact_database: | ||
| artifact_database[tool_call_id] = artifacts | ||
| else: | ||
| artifact_database[tool_call_id].extend(artifacts) | ||
| with open("artifact_database.pkl", "wb") as file: | ||
| with path.open("wb") as file: | ||
| pickle.dump(artifact_database, file) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,12 @@ | |
|
|
||
|
|
||
| def run_with_timeout(cmd: List[str], timeout_sec: int): | ||
| if isinstance(timeout_sec, bool) or not isinstance(timeout_sec, (int, float)): | ||
| raise TypeError( | ||
| f"timeout_sec must be a positive number, got {type(timeout_sec).__name__}" | ||
| ) | ||
| if timeout_sec <= 0: | ||
| raise ValueError(f"timeout_sec must be positive, got {timeout_sec!r}") | ||
|
Comment on lines
+25
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: sed -n '1,220p' src/rai_core/rai/tools/ros2/cli.pyRepository: RobotecAI/rai Length of output: 6071 🏁 Script executed: python3 - <<'PY'
import math
from threading import Timer
vals = [float('nan'), float('inf'), float('-inf'), 0.1, -1.0, 1]
for v in vals:
try:
print(type(v).__name__, v, 'finite=', math.isfinite(v), '<=0=', (v <= 0))
except Exception as e:
print(type(v).__name__, v, 'error comparing:', e)
# Probe Timer construction/start behavior with NaN/inf without executing callbacks long-term.
def cb():
print("callback fired")
for v in [float('nan'), float('inf'), 0.001]:
t = Timer(v, cb)
print("Timer", v, "interval attr:", t.interval)
PYRepository: RobotecAI/rai Length of output: 445 🏁 Script executed: python3 - <<'PY'
import math
from threading import Timer
vals = [float('nan'), float('inf'), float('-inf'), 0.1, -1.0, 1]
for v in vals:
print(type(v).__name__, v, 'finite=', math.isfinite(v), '<=0=', (v <= 0))
for v in [float('nan'), float('inf'), 0.001]:
t = Timer(v, lambda: None)
print("Timer", v, "interval attr:", t.interval)
PYRepository: RobotecAI/rai Length of output: 445 🏁 Script executed: python3 - <<'PY'
import time
from threading import Timer
results = []
def mark(label):
results.append((label, time.monotonic()))
start = time.monotonic()
for label, interval in [("nan", float("nan")), ("inf", float("inf")), ("short", 0.05)]:
t = Timer(interval, mark, args=(label,))
t.daemon = True
t.start()
time.sleep(0.2)
print("elapsed", round(time.monotonic() - start, 3))
print("results", results)
PYRepository: RobotecAI/rai Length of output: 826 Reject non-finite timeout values. 🤖 Prompt for AI Agents |
||
| proc = Popen(cmd, stdout=PIPE, stderr=PIPE) | ||
| timer = Timer(timeout_sec, proc.kill) | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| # Copyright (C) 2025 Robotec.AI | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import pytest | ||
| from rai.agents.langchain.callback import HRICallbackHandler | ||
|
|
||
|
|
||
| def test_hri_callback_handler_rejects_nonpositive_max_buffer_size(): | ||
| with pytest.raises(ValueError, match="max_buffer_size must be positive"): | ||
| HRICallbackHandler(connectors={}, max_buffer_size=0) | ||
| with pytest.raises(ValueError, match="max_buffer_size must be positive"): | ||
| HRICallbackHandler(connectors={}, max_buffer_size=-1) | ||
|
|
||
|
|
||
| def test_hri_callback_handler_rejects_non_int_max_buffer_size(): | ||
| with pytest.raises(TypeError, match="max_buffer_size must be a positive int"): | ||
| HRICallbackHandler(connectors={}, max_buffer_size=False) | ||
| with pytest.raises(TypeError, match="max_buffer_size must be a positive int"): | ||
| HRICallbackHandler(connectors={}, max_buffer_size=1.5) # type: ignore[arg-type] | ||
| with pytest.raises(TypeError, match="max_buffer_size must be a positive int"): | ||
| HRICallbackHandler(connectors={}, max_buffer_size="200") # type: ignore[arg-type] | ||
|
|
||
|
|
||
| def test_hri_callback_handler_accepts_positive_max_buffer_size(): | ||
| handler = HRICallbackHandler(connectors={}, max_buffer_size=1) | ||
| assert handler.max_buffer_size == 1 | ||
| default_handler = HRICallbackHandler(connectors={}) | ||
| assert default_handler.max_buffer_size == 200 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # Copyright (C) 2025 Robotec.AI | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
| from rai.agents.langchain.agent import LangChainAgent | ||
|
|
||
|
|
||
| def _make_agent(max_size: int) -> LangChainAgent: | ||
| return LangChainAgent( | ||
| target_connectors={}, | ||
| runnable=MagicMock(), | ||
| max_size=max_size, | ||
| ) | ||
|
|
||
|
|
||
| def test_max_size_must_be_positive(): | ||
| for bad in (0, -1, False): | ||
| with pytest.raises(ValueError, match="max_size must be positive"): | ||
| _make_agent(bad) # type: ignore[arg-type] | ||
|
|
||
|
|
||
| def test_max_size_accepts_positive(): | ||
| agent = _make_agent(1) | ||
| assert agent.max_size == 1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Copyright (C) 2025 Robotec.AI | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import pytest | ||
| from pydantic import ValidationError | ||
| from rai.agents.langchain.state_based_agent import StateBasedConfig | ||
|
|
||
|
|
||
| def test_time_interval_must_be_positive(): | ||
| for bad in (0, -1.0, 0.0): | ||
| with pytest.raises(ValidationError): | ||
| StateBasedConfig(aggregators={}, time_interval=bad) | ||
|
|
||
|
|
||
| def test_max_workers_must_be_positive(): | ||
| for bad in (0, -1): | ||
| with pytest.raises(ValidationError): | ||
| StateBasedConfig(aggregators={}, max_workers=bad) | ||
|
|
||
|
|
||
| def test_defaults_and_positive_ok(): | ||
| cfg = StateBasedConfig(aggregators={}) | ||
| assert cfg.time_interval == 5.0 | ||
| assert cfg.max_workers == 8 | ||
| cfg2 = StateBasedConfig(aggregators={}, time_interval=0.1, max_workers=1) | ||
| assert cfg2.time_interval == 0.1 | ||
| assert cfg2.max_workers == 1 |
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: RobotecAI/rai
Length of output: 11112
🏁 Script executed:
Repository: RobotecAI/rai
Length of output: 9010
🏁 Script executed:
Repository: RobotecAI/rai
Length of output: 1310
Reject non-integer
max_sizevalues.max_sizeis documented and typed as anint, butTrueand1.5still pass this guard. Require a non-bool positiveintand add regression tests for those cases.🤖 Prompt for AI Agents