chore(types): Type-clean /actions (189 errors) #1361

tgasser-nv · 2025-08-29T04:08:09Z

Description

Nemo Guardrails doesn't have type-checking enforced in the project today. This PR will be used to compile type-cleaning related to the nemoguardrails/actions module prior to merging into the top-level type-cleaning PR1367.

Progress

Prior to this work the actions module had 189 errors, 0 warnings, and 0 informations.

Related Issue(s)

Checklist

I've read the CONTRIBUTING guidelines.
I've updated the documentation if applicable.
I've added tests if applicable.
@mentions of the person or team responsible for reviewing proposed changes.

codecov-commenter · 2025-08-29T04:13:31Z

Codecov Report

❌ Patch coverage is 84.09091% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.62%. Comparing base (9a1d178) to head (74026bc).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
nemoguardrails/actions/llm/generation.py	82.71%	14 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1361      +/-   ##
===========================================
- Coverage    71.68%   71.62%   -0.07%     
===========================================
  Files          168      171       +3     
  Lines        16862    17053     +191     
===========================================
+ Hits         12088    12214     +126     
- Misses        4774     4839      +65

Flag	Coverage Δ
python	`71.62% <84.09%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
nemoguardrails/actions/llm/utils.py	`80.00% <100.00%> (ø)`
nemoguardrails/context.py	`100.00% <100.00%> (ø)`
nemoguardrails/actions/llm/generation.py	`85.00% <82.71%> (-0.96%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cparisien · 2025-09-05T16:44:00Z

@tgasser-nv, any guidance on how to review this? I'm struggling a bit. If we're more strongly enforcing types, do we mainly rely on test coverage to ensure we haven't broken anything with the new constraints?

tgasser-nv · 2025-09-05T19:26:29Z

@tgasser-nv, any guidance on how to review this? I'm struggling a bit. If we're more strongly enforcing types, do we mainly rely on test coverage to ensure we haven't broken anything with the new constraints?

Yes, I run the unit-tests continuously as I'm cleaning Pyright errors. We have far too many optional types in our Pydantic models and function/method signatures, the biggest bucket of fixes are asserting these aren't None before going on to use them.

The lines which are either too complex to fix (i.e. generation.py prompt assignments with 10 levels of nested indentation) or too hard to understand (what is this line doing?!) are marked with # pyright: ignore and a TODO relating to them.

tgasser-nv · 2025-09-05T19:29:05Z

Status: Pyright errors: 0, waived: 7.
Debugging test_passthrough_llm_action_invoked_via_logs

tgasser-nv · 2025-09-06T15:07:42Z

Fixing the types exposed a bug in an untested function.

The test_passthrough_llm_action_invoked_via_logs test fails because ther serialization.py state_to_json() function doesn't correctly serialize ParsedTaskOutput. I added a skip decorator and created Github issue #1378 to track the work to fix this.

>       raise TypeError(f'Object of type {o.__class__.__name__} '
                        f'is not JSON serializable')
E       TypeError: Object of type ParsedTaskOutput is not JSON serializable

…mo_guardrails/nemoguardrails/actions/llm/generation.py:537:40 - error: Never is not awaitable (reportGeneralTypeIssues)

nemoguardrails/actions/llm/utils.py

codecov-commenter · 2025-09-25T20:27:40Z

Codecov Report

❌ Patch coverage is 81.02564% with 37 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
nemoguardrails/actions/v2_x/generation.py	54.54%	20 Missing ⚠️
nemoguardrails/actions/llm/generation.py	83.95%	13 Missing ⚠️
nemoguardrails/actions/action_dispatcher.py	94.44%	2 Missing ⚠️
nemoguardrails/actions/llm/utils.py	92.30%	1 Missing ⚠️
...emoguardrails/actions/validation/filter_secrets.py	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

nemoguardrails/actions/llm/generation.py

Pouyanpi · 2025-09-26T08:57:53Z

nemoguardrails/actions/llm/generation.py

+        assert event
        assert event["type"] == "UserMessage"


not related to your changes, but should not use assert

if not event: raise ValueError( "No user message found in event stream. Unable to generate user intent." ) if event["type"] != "UserMessage": raise ValueError( f"Expected UserMessage event, but found {event['type']}. " "Cannot generate user intent from this event type." )

Yeah I agree. I wasn't sure if there was a good reason to use assert here rather than raising exceptions and wanted to be consistent with existing code. Will make this change.

nemoguardrails/actions/llm/generation.py

Pouyanpi · 2025-09-26T09:00:22Z

nemoguardrails/actions/llm/generation.py

+        assert event
        assert event["type"] == "UserMessage"


same as above we should not use assert in our non-test codes

Yep agree here, added your sample code in from the above comment for this one too. Not worrying about DRYing the code too much since this file has three functions with untestable CC before refactoring:

/Users/tgasser/projects/nemo_guardrails/nemoguardrails/actions/llm/generation.py M 1235:4 LLMGenerationActions.generate_intent_steps_message - F (50) M 864:4 LLMGenerationActions.generate_bot_message - F (43) M 381:4 LLMGenerationActions.generate_user_intent - F (42)

I'll DRY this up when refactoring to address the Cyclomatic Complexity

nemoguardrails/actions/llm/generation.py

Pouyanpi · 2025-09-26T09:18:36Z

nemoguardrails/actions/v2_x/generation.py

+            results = (
+                await self.flows_index.search(
+                    text=f"${var_name} = ", max_results=5, threshold=None
                )
+                if var_name
+                else None
+            )


nit: To me in line if-else in python is less readable

if var_name: results = await self.flows_index.search( text=f"${var_name} = ", max_results=5, threshold=None ) else: results = None

but no need to make this change, it is just a preference 👍🏻

Agree, I reworked this to use the same structure as above

Pouyanpi · 2025-09-26T09:27:50Z

nemoguardrails/actions/v2_x/generation.py

+            )
+
+            if not results:
+                raise Exception("No results found while generating value")


I am not sure about this Exception

I think None value should be OK, the system should generate a value based on instructions alone, without needing similar examples from the flows index.

Sounds good, I added a guard statement before the examples string append loop:

# We add these in reverse order so the most relevant is towards the end. if results: for result in reversed(results): # If the flow includes "GenerateValueAction", we ignore it as we don't want the LLM # to learn to predict it. if "GenerateValueAction" not in result.text: examples += f"{result.text}\n\n"

Pouyanpi · 2025-09-26T09:35:32Z

nemoguardrails/actions/v2_x/generation.py

        triggering_flow_id = flow_id
+        if not triggering_flow_id:
+            raise Exception(
+                f"No flow_id provided to generate flow."


I am not sure

Suggested change

f"No flow_id provided to generate flow."

"No flow_id provided to generate flow."

Pouyanpi · 2025-09-26T09:42:10Z

nemoguardrails/actions/action_dispatcher.py

-                relative_filepath = Path(module.__file__).relative_to(Path.cwd())
-            except ValueError:
-                relative_filepath = Path(module.__file__).resolve()
+            # todo! What are we trying to do here?


😆

I think the intent was to make it easier to traceback the problematic file.
So if there are multiple identical filenames, which one is problematic

Makes sense, I added this back in again with some extra checking to make it type-clean

nemoguardrails/actions/actions.py

Copilot

Pull Request Overview

This PR performs type-cleaning for the NeMo Guardrails actions module to address 189 type errors identified prior to enforcing type-checking. The changes include adding type annotations, import fixes, handling optional types, and adding necessary tests to cover edge cases.

Adds comprehensive type annotations throughout the actions module
Updates imports to use proper langchain-core modules and adds type checking compatibility
Improves error handling and null checks in action dispatcher and generation functions

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
nemoguardrails/actions/actions.py	Adds Protocol definitions and improved type annotations for action decorators
nemoguardrails/actions/action_dispatcher.py	Enhances type safety with better annotations and error handling for action execution
nemoguardrails/actions/llm/generation.py	Major type improvements for LLM generation actions with proper optional handling
nemoguardrails/actions/llm/utils.py	Updates LLM utility functions with enhanced type annotations and null checks
nemoguardrails/actions/v2_x/generation.py	Type-safe improvements for v2.x generation actions
tests/	Adds comprehensive test coverage for previously untested edge cases
pyproject.toml	Enables type checking for the actions module

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-26T09:56:36Z

nemoguardrails/actions/llm/generation.py

+                )
+
                result = await llm_call(
                    llm,


The variable llm is used here but should be generation_llm to match the pattern used elsewhere in the function and maintain type consistency.

Suggested change

llm,

generation_llm,

This is a good catch, applied it locally and checked it with unit-tests

Copilot · 2025-09-26T09:56:37Z

nemoguardrails/actions/v2_x/generation.py

+from langchain_core.language_models import BaseChatModel
 from langchain_core.language_models.llms import BaseLLM
+from langchain_text_splitters import ElementType
+from pytest_asyncio.plugin import event_loop


This import appears to be a development/testing dependency that shouldn't be in production code. This import should be removed as it's not used in the file and pytest_asyncio is a test-only dependency.

Suggested change

from pytest_asyncio.plugin import event_loop

@tgasser-nv

There are some unsused imports including the pytest_asynci

from langchain_text_splitters import ElementType from pytest_asyncio.plugin import event_loop

and

get_initial_actions,

Removed these unused imports

nemoguardrails/actions/llm/generation.py

Copilot · 2025-09-26T09:56:37Z

nemoguardrails/actions/v2_x/generation.py

        triggering_flow_id = flow_id
+        if not triggering_flow_id:
+            raise Exception(
+                f"No flow_id provided to generate flow."


The error message uses an f-string but doesn't include any variables. The message should be a plain string: \"No flow_id provided to generate flow.\"

Suggested change

f"No flow_id provided to generate flow."

"No flow_id provided to generate flow."

nemoguardrails/actions/v2_x/generation.py

nemoguardrails/actions/action_dispatcher.py

Pouyanpi · 2025-09-26T10:59:48Z

nemoguardrails/actions/v2_x/generation.py

+
        event = get_last_user_utterance_event_v2_x(events)
+        if not event:
+            raise Exception("Passthrough LLM Action couldn't find last user utterance")


nit: better to avoid general Exception

Agree, replaced with RuntimeError

Pouyanpi · 2025-09-26T11:00:25Z

nemoguardrails/actions/v2_x/generation.py

        log.info("Generating flow for name: {name}")

+        if not self.instruction_flows_index:
+            raise Exception("No instruction flows index has been created.")


RuntimeError maybe

Yep updated this.

Pouyanpi · 2025-09-26T11:18:47Z

nemoguardrails/actions/actions.py

+class Actionable(Protocol):
+    """Protocol for any object with ActionMeta metadata (i.e. decorated with @action)"""


The Protocol is not used and adds unnecessary complexity. We're already doing runtime attribute checking with hasattr(obj, 'action_meta') throughout the codebase. maybe a util function like is_action. duck typing

I got rid of the Protocol, I think I tried to solve the type issues with this and couldn't get it working. That's when I went back to checking attributes dynamically. I removed this

Pouyanpi

@tgasser-nv, thanks, looks good! Please merge after addressing the comments :+1

tgasser-nv · 2025-09-26T21:46:27Z

I addressed all feedback, and unit-tests are passing locally. For some reason the Github action Python 3.10 unit-tests are crashing with Fatal Python error: Illegal instruction. I can't debug any further because I can't get to the machine the actions run on.

I ran locally and all the unit-tests passed on Python 3.10 locally. Full logs here:
20250926_pytest_py3.10.18.log

============================= test session starts ==============================
platform darwin -- Python 3.10.18, pytest-8.4.1, pluggy-1.6.0 -- /Users/tgasser/Library/Caches/pypoetry/virtualenvs/nemoguardrails-qkVbfMSD-py3.10/bin/python
cachedir: .pytest_cache
rootdir: /Users/tgasser/projects/nemo_guardrails
configfile: pytest.ini
testpaths: tests, docs/colang-2/examples
plugins: httpx-0.35.0, anyio-4.10.0, langsmith-0.4.20, cov-6.2.1, asyncio-0.26.0, profiling-1.8.1
asyncio: mode=strict, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
collecting ... collected 1681 items

tests/colang/parser/test_basic.py::test_1 PASSED                         [  0%]
tests/colang/parser/test_basic.py::test_2 PASSED                         [  0%
tests/colang/parser/test_basic.py::test_3 PASSED                         [  0%]
tests/colang/parser/v2_x/test_ast.py::test_basic PASSED                  [  0%]
....

tests/v2_x/test_various_mechanics.py::test_expr_func_search PASSED       [ 99%]
tests/v2_x/test_various_mechanics.py::test_generate_value_with_NLD PASSED [ 99%]
tests/v2_x/test_various_mechanics.py::test_flow_states_info PASSED       [100%]

================ 1573 passed, 108 skipped in 137.96s (0:02:17) =================

With this I'm going to bypass the Python 3.10 unit-test result and merge.

Type-cleaned all files under `nemoguard/actions` and added them to pyright pre-commit hooks so type-coverage doesn't regress.

tgasser-nv requested review from trebedea, Pouyanpi and cparisien August 29, 2025 04:08

tgasser-nv self-assigned this Aug 29, 2025

tgasser-nv changed the title ~~chore: Type-fixes for generation.py~~ chore: Guardrails Type-fixes Sep 2, 2025

tgasser-nv changed the base branch from develop to chore/type-clean-guardrails September 2, 2025 16:19

tgasser-nv changed the title ~~chore: Guardrails Type-fixes~~ chore: Guardrails actions type-fixes Sep 2, 2025

tgasser-nv marked this pull request as draft September 2, 2025 16:22

tgasser-nv changed the title ~~chore: Guardrails actions type-fixes~~ chore(types): Guardrails actions module type-fixes Sep 5, 2025

tgasser-nv mentioned this pull request Sep 6, 2025

chore(types): Top-level PR for Guardrails type-cleaning (925 errors) #1367

Open

19 tasks

tgasser-nv marked this pull request as ready for review September 6, 2025 15:05

tgasser-nv changed the title ~~chore(types): Guardrails actions module type-fixes~~ chore(types): Type-clean /actions Sep 8, 2025

tgasser-nv changed the title ~~chore(types): Type-clean /actions~~ chore(types): Type-clean /actions (189 errors) Sep 10, 2025

tgasser-nv changed the base branch from chore/type-clean-guardrails to develop September 22, 2025 21:31

tgasser-nv force-pushed the chore/type-clean-actions-llm-generation branch from 5fb3780 to b751596 Compare September 24, 2025 19:14

tgasser-nv added 10 commits September 25, 2025 15:10

Cleaned _extract_user_message_example

00032d8

Cleaned _extract_bot_message_example

0506a5f

Cleaned _process_flows()

437ead1

Cleaned _get_general_instructions and _get_sample_conversation_two_turns

f073992

Cleaned generate_user_intent()

bf89bc2

Cleaned generate_user_intent()

0c5fdb5

Cleaned generate_user_intent() apart from: /Users/tgasser/projects/ne…

c3e73fc

…mo_guardrails/nemoguardrails/actions/llm/generation.py:537:40 - error: Never is not awaitable (reportGeneralTypeIssues)

Cleaned _search_flows_index()

fbb0e75

Clean generate_next_step()

f9fb809

Fix llm input argument and variable shadowing in generate_user_intent()

4cf304e

tgasser-nv commented Sep 25, 2025

View reviewed changes

nemoguardrails/actions/llm/utils.py Show resolved Hide resolved

tgasser-nv commented Sep 25, 2025

View reviewed changes

nemoguardrails/actions/llm/utils.py Show resolved Hide resolved

Re-ran pre-commits after a rebase+force push

96aafc6

tgasser-nv added 2 commits September 25, 2025 17:06

Add unit-tests to cover action_dispatcher.py patch coverage gaps

f7cbcf8

Patch coverage improvements

281afc8

Pouyanpi reviewed Sep 26, 2025

View reviewed changes

nemoguardrails/actions/llm/generation.py Show resolved Hide resolved

Pouyanpi reviewed Sep 26, 2025

View reviewed changes

nemoguardrails/actions/llm/generation.py Show resolved Hide resolved

Pouyanpi reviewed Sep 26, 2025

View reviewed changes

nemoguardrails/actions/llm/generation.py Show resolved Hide resolved

Pouyanpi reviewed Sep 26, 2025

View reviewed changes

nemoguardrails/actions/llm/generation.py Show resolved Hide resolved

Pouyanpi reviewed Sep 26, 2025

View reviewed changes

nemoguardrails/actions/actions.py Show resolved Hide resolved

Pouyanpi requested a review from Copilot September 26, 2025 09:54

Copilot AI reviewed Sep 26, 2025

View reviewed changes

Pouyanpi reviewed Sep 26, 2025

View reviewed changes

Pouyanpi approved these changes Sep 26, 2025

View reviewed changes

Address Pouyan's feedback

8ebbc22

tgasser-nv merged commit 67de947 into develop Sep 26, 2025
27 of 32 checks passed

tgasser-nv deleted the chore/type-clean-actions-llm-generation branch September 26, 2025 21:47

Pouyanpi pushed a commit that referenced this pull request Sep 29, 2025

chore(types): Type-clean /actions (189 errors) (#1361)

99b83ca

Type-cleaned all files under `nemoguard/actions` and added them to pyright pre-commit hooks so type-coverage doesn't regress.

Pouyanpi pushed a commit that referenced this pull request Oct 1, 2025

chore(types): Type-clean /actions (189 errors) (#1361)

d040359

Type-cleaned all files under `nemoguard/actions` and added them to pyright pre-commit hooks so type-coverage doesn't regress.

gautamvarmadatla mentioned this pull request Oct 4, 2025

bug: state_to_json() doesn't correctly serialize dataclasses #1378

Open

4 tasks

	f"No flow_id provided to generate flow."
	"No flow_id provided to generate flow."

		class Actionable(Protocol):
		"""Protocol for any object with ActionMeta metadata (i.e. decorated with @action)"""

chore(types): Type-clean /actions (189 errors) #1361

chore(types): Type-clean /actions (189 errors) #1361

Conversation

tgasser-nv commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Progress

Related Issue(s)

Checklist

Uh oh!

codecov-commenter commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cparisien commented Sep 5, 2025

Uh oh!

tgasser-nv commented Sep 5, 2025

Uh oh!

tgasser-nv commented Sep 5, 2025

Uh oh!

tgasser-nv commented Sep 6, 2025

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pouyanpi Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

tgasser-nv commented Aug 29, 2025 •

edited

Loading

codecov-commenter commented Aug 29, 2025 •

edited

Loading

codecov-commenter commented Sep 25, 2025 •

edited

Loading

Pouyanpi Sep 26, 2025 •

edited

Loading

tgasser-nv commented Sep 26, 2025 •

edited

Loading