[ONGOING, DO NOT MERGE] Fixing issues in verify_graph task - #290
[ONGOING, DO NOT MERGE] Fixing issues in verify_graph task#290NiveditJain wants to merge 1 commit into
Conversation
- Updated verification functions to return lists of errors instead of modifying an external list, improving function clarity and usability. - Introduced asyncio.gather for concurrent execution of verification tasks in verify_graph, enhancing performance. - Adjusted function signatures to reflect the new return types, ensuring consistency across the module. These changes streamline the error handling process and optimize the verification workflow.
📝 WalkthroughSummary by CodeRabbit
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @NiveditJain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the verify_graph task to enhance its error handling and optimize performance. The core change involves modifying individual verification functions to return lists of errors directly, rather than relying on an external mutable list. Additionally, asyncio.gather has been introduced to enable concurrent execution of these verification sub-tasks, leading to improved efficiency. These changes streamline the error reporting mechanism and accelerate the graph validation process.
Highlights
- Improved Error Handling: Verification functions now return lists of errors, enhancing modularity and clarity by removing the dependency on an external error list.
- Performance Optimization: Implemented
asyncio.gatherto concurrently execute multiple graph verification sub-tasks, significantly speeding up the overallverify_graphprocess. - API Consistency: Updated function signatures across the module to reflect the new return types, ensuring a consistent and predictable interface for verification utilities.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request makes excellent progress in refactoring the graph verification tasks. The move to pure functions that return errors instead of modifying an external list is a great improvement for code clarity and testability. The introduction of asyncio.gather is also a smart choice for improving performance by running verification tasks concurrently.
I've identified a critical issue with how the results from asyncio.gather are being processed, which will lead to incorrect error formatting. Additionally, the refactoring seems incomplete, as a couple of verification functions still follow the old pattern of modifying a passed-in list. I've also left a few minor comments regarding code style.
Addressing these points will help ensure the refactoring is robust, consistent, and fully achieves its goals.
| verify_node_identifiers(graph_template.nodes), | ||
| verify_secrets(graph_template, database_nodes) | ||
| ] | ||
| errors.extend(await asyncio.gather(*basic_verify_tasks)) |
There was a problem hiding this comment.
The result of asyncio.gather(*basic_verify_tasks) is a list of lists of strings (e.g., [['error1'], [], ['error2']]), as each verification task returns a list[str]. Using errors.extend() here will create a nested list of errors, which is not the expected flat list[str] for graph_template.validation_errors. This will cause incorrect error reporting.
You should flatten the list of lists into a single list of error strings.
| errors.extend(await asyncio.gather(*basic_verify_tasks)) | |
| errors.extend([error for error_list in await asyncio.gather(*basic_verify_tasks) for error in error_list]) |
| for node in nodes: | ||
| if node.node_name is None or node.node_name == "": | ||
| errors.append(f"Node {node.identifier} has no name") | ||
| errors.append(f"Node {node.identifier} has no name") |
There was a problem hiding this comment.
This line has trailing whitespace, which should be removed to adhere to the PEP 8 style guide.
| errors.append(f"Node {node.identifier} has no name") | |
| errors.append(f"Node {node.identifier} has no name") |
| database_nodes_set = set([(node.name, node.namespace) for node in database_nodes]) | ||
|
|
||
| nodes_not_found = template_nodes_set - database_nodes_set | ||
|
|
There was a problem hiding this comment.
This line contains only whitespace and creates an unnecessary blank line. According to the PEP 8 style guide (E303), excessive blank lines should be avoided. Please remove this line.
| dependency_graph = await verify_topology(graph_template.nodes, errors) | ||
|
|
||
| if dependency_graph is not None and len(errors) == 0: | ||
| await verify_inputs(graph_template.nodes, database_nodes, dependency_graph, errors) | ||
| inputs_errors = await verify_inputs(graph_template.nodes, database_nodes, dependency_graph) | ||
| errors.extend(inputs_errors) | ||
|
|
||
| await verify_unites(graph_template.nodes, dependency_graph, errors) |
There was a problem hiding this comment.
The refactoring to make verification functions pure is incomplete. The verify_topology and verify_unites functions still modify the errors list passed as an argument, which is inconsistent with the new pattern adopted for other verification functions in this pull request.
For consistency and to fully realize the benefits of this refactoring, these functions should also be updated to return a list of errors instead of modifying the list in-place.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
state-manager/app/tasks/verify_graph.py (6)
37-67: Identifier validation: add optionalerrorsparam and keep behaviorPrevents test failures due to signature mismatch; also removes local name collision.
-async def verify_node_identifiers(nodes: list[NodeTemplate]) -> list[str]: - errors = [] +async def verify_node_identifiers(nodes: list[NodeTemplate], errors: list[str] | None = None) -> list[str]: + errs: list[str] = [] @@ - if node.identifier is None or node.identifier == "": - errors.append(f"Node {node.node_name} in namespace {node.namespace} has no identifier") + if node.identifier is None or node.identifier == "": + errs.append(f"Node {node.node_name} in namespace {node.namespace} has no identifier") continue @@ - errors.append(f"Duplicate identifier '{identifier}' found in nodes: {node_list}") + errs.append(f"Duplicate identifier '{identifier}' found in nodes: {node_list}") @@ - if next_node not in valid_identifiers: - errors.append(f"Node {node.node_name} in namespace {node.namespace} has a next node {next_node} that does not exist in the graph") + if next_node not in valid_identifiers: + errs.append(f"Node {node.node_name} in namespace {node.namespace} has a next node {next_node} that does not exist in the graph") @@ - return errors + if errors is not None: + errors.extend(errs) + return errs
68-88: Secrets validation: add optionalerrorsparam and guard whensecretsis emptyThis maintains back-compat and avoids
AttributeErrorifgraph_template.secretsis empty or None-like.-async def verify_secrets(graph_template: GraphTemplate, database_nodes: list[RegisteredNode]) -> list[str]: - errors = [] +async def verify_secrets(graph_template: GraphTemplate, database_nodes: list[RegisteredNode], errors: list[str] | None = None) -> list[str]: + errs: list[str] = [] @@ - errors.append(f"Secret {secret_name} is required but not present in the graph template") - - return errors + errs.append(f"Secret {secret_name} is required but not present in the graph template") + + if errors is not None: + errors.extend(errs) + return errsAnd adjust the present-secrets collection within this hunk:
- present_secrets_set = set() - for secret_name in graph_template.secrets.keys(): - present_secrets_set.add(secret_name) + present_secrets_set = set((graph_template.secrets or {}).keys())
107-165: Harden input validation: guard missing DB node and non-string inputs; guard referenced output DB nodeTwo crashers here:
look_up_table[node.identifier]["database_node"]may be missing if registration is absent, causingKeyError.- Splitting inputs assumes values are strings; non-string values will raise
AttributeError.
Also guard the referenced node’s DB registration before building its outputs model.Apply this diff:
@@ - for node in graph_nodes: - try: - model_class = create_model(look_up_table[node.identifier]["database_node"].inputs_schema) + for node in graph_nodes: + try: + db_node = look_up_table[node.identifier].get("database_node") + if db_node is None: + errors.append(f"No registered database node found for {node.node_name} in namespace {node.namespace}") + continue + model_class = create_model(db_node.inputs_schema) @@ - if field_name not in look_up_table[node.identifier]["graph_node"].inputs.keys(): + if field_name not in look_up_table[node.identifier]["graph_node"].inputs.keys(): errors.append(f"{node.node_name}.Inputs field '{field_name}' not found in graph template") continue @@ - splits = look_up_table[node.identifier]["graph_node"].inputs[field_name].split("${{") + value = look_up_table[node.identifier]["graph_node"].inputs[field_name] + if not isinstance(value, str): + errors.append(f"{node.node_name}.Inputs field '{field_name}' must be a string, got {type(value).__name__}") + continue + splits = value.split("${{") @@ - output_model_class = create_model(look_up_table[identifier]["database_node"].outputs_schema) + ref_entry = look_up_table.get(identifier, {}) + ref_db_node = ref_entry.get("database_node") + if ref_db_node is None: + errors.append(f"{node.node_name}.Inputs field '{field_name}' references node {identifier} which is not registered in database") + continue + output_model_class = create_model(ref_db_node.outputs_schema)
166-176: Dead code or unused API:build_dependencies_graphcurrently unused and returns setsThis function returns
dict[str, set[str]]but the rest of the pipeline uses the DFS-built dependency list. Either remove it to avoid confusion or switchverify_inputsto consume this (convert sets to lists) and use it consistently.Would you like me to wire this in (and adapt
verify_inputsaccordingly), or remove it in this PR to reduce maintenance surface?
208-227: Fix mutable default argument in DFS helper
current_path: list[str] = []shares state across invocations. Make itNonewith an in-function initializer.- def dfs_visit(current_node: str, parent_node: str | None = None, current_path: list[str] = []): + def dfs_visit(current_node: str, parent_node: str | None = None, current_path: list[str] | None = None): + if current_path is None: + current_path = []
265-271: Gateverify_uniteson successful topology/basic checks to avoid KeyErrorIf the graph is disconnected, some identifiers may not be present in
dependency_graph, andverify_unitescan raise before we persisterrors. Only run unites when there are no errors and a dependency graph exists.- await verify_unites(graph_template.nodes, dependency_graph, errors) + if dependency_graph is not None and not errors: + await verify_unites(graph_template.nodes, dependency_graph, errors)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
state-manager/app/tasks/verify_graph.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state-manager/app/tasks/verify_graph.py (3)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(15-124)
🪛 GitHub Actions: State Manager Unit Tests
state-manager/app/tasks/verify_graph.py
[error] 1-1: verify_nodes_names() signature mismatch. Defined to take 1 positional argument, but tests call with 2 (nodes, errors).
[error] 1-1: verify_nodes_namespace() signature mismatch. Defined to take 2 positional arguments, but tests call with 3 (nodes, namespace, errors).
[error] 1-1: verify_node_exists() signature mismatch. Defined to take 2 positional arguments, but tests call with 3 (nodes, database_nodes, errors).
[error] 1-1: verify_node_identifiers() signature mismatch. Defined to take 1 positional argument, but tests call with 2 (nodes, errors).
[error] 1-1: verify_secrets() signature mismatch. Defined to take 2 positional arguments, but tests call with 3 (graph_template, database_nodes, errors).
🔇 Additional comments (1)
state-manager/app/tasks/verify_graph.py (1)
89-105: DB lookups LGTMQuerying both namespaces and concatenating results is clear and typed. No change requested.
| async def verify_nodes_names(nodes: list[NodeTemplate]) -> list[str]: | ||
| errors = [] | ||
| for node in nodes: | ||
| if node.node_name is None or node.node_name == "": | ||
| errors.append(f"Node {node.identifier} has no name") | ||
| errors.append(f"Node {node.identifier} has no name") | ||
| return errors |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add backward-compatible optional errors parameter and avoid shadowing
Tests are still calling the old signature with (nodes, errors). Accept an optional errors param and, if provided, extend it for backward compatibility. Also avoid shadowing by using a different local variable name.
-async def verify_nodes_names(nodes: list[NodeTemplate]) -> list[str]:
- errors = []
+async def verify_nodes_names(nodes: list[NodeTemplate], errors: list[str] | None = None) -> list[str]:
+ errs: list[str] = []
for node in nodes:
- if node.node_name is None or node.node_name == "":
- errors.append(f"Node {node.identifier} has no name")
- return errors
+ if not node.node_name:
+ errs.append(f"Node {node.identifier} has no name")
+ if errors is not None:
+ errors.extend(errs)
+ return errsRun this to confirm all call-sites now work with the optional param (both styles supported):
🏁 Script executed:
#!/bin/bash
# Find invocations providing an 'errors' arg (legacy signature)
rg -nP '\bverify_nodes_names\s*\(\s*[^,]+,\s*[^)]+\)'Length of output: 364
Backward Compatibility for verify_nodes_names
The tests in state-manager/tests/unit/tasks/test_verify_graph.py invoke:
await verify_nodes_names(nodes, errors)at lines 34 and 47await verify_nodes_names(nodes, errors)again at line 88
These calls rely on two behaviors of the original signature:
- Passing in an existing
errorslist and having it populated. - Receiving the same
errorslist back as the return value.
The proposed diff instead:
- Introduces a new local
errslist. - Extends the passed-in
errorslist (if non-null) but returns onlyerrs.
This breaks callers expecting:
errors = []
result = await verify_nodes_names(nodes, errors)
# result is errors, and errors has been mutatedWith the new code, result is a distinct list (errs), so result is not errors.
To restore full backward compatibility:
- Use the passed-in
errorslist as the working list when provided. - Only allocate a new list when
errorsisNone. - Return the very list that callers passed in (or the new one when none was passed).
Suggested replacement:
- async def verify_nodes_names(nodes: list[NodeTemplate], errors: list[str] | None = None) -> list[str]:
- errs: list[str] = []
- for node in nodes:
- if not node.node_name:
- errs.append(f"Node {node.identifier} has no name")
- if errors is not None:
- errors.extend(errs)
- return errs
+ async def verify_nodes_names(
+ nodes: list[NodeTemplate],
+ errors: list[str] | None = None,
+ ) -> list[str]:
+ # Use provided list for backward compatibility, or create one if omitted
+ error_list = errors if errors is not None else []
+
+ for node in nodes:
+ if not node.node_name:
+ error_list.append(f"Node {node.identifier} has no name")
+
+ return error_listThis ensures:
- If callers pass
errors, it's both mutated and returned. - If callers omit
errors, a new list is returned.
Please update the implementation accordingly to fully satisfy existing tests.
🤖 Prompt for AI Agents
In state-manager/app/tasks/verify_graph.py around lines 12 to 17, the function
now creates and returns a new local list instead of using and returning the
caller-provided errors list, breaking callers who expect the same list to be
mutated and returned; change the implementation to accept an optional errors
parameter, set errors = errors or [] (i.e. only allocate when None), append
validation messages to that errors list, and return that same errors list so
callers receive and observe mutations to the original list.
💡 Verification agent
🧩 Analysis chain
Resolve pipeline signature mismatches with a temporary compatibility shim
Given the CI failures, the optional errors parameter pattern above should make both legacy tests and the new return-based style pass. Recommend adding a deprecation note and removing the param after tests are migrated.
Run this to find and update tests calling the legacy signatures:
I can submit a follow-up patch to update the tests to the new return-based API and then remove the shims.
Also applies to: 19-25, 26-36, 37-67, 68-88
🏁 Script executed:
#!/bin/bash
# Search legacy-style invocations that pass an explicit 'errors' list
rg -nP '\b(verify_nodes_names|verify_nodes_namespace|verify_node_exists|verify_node_identifiers|verify_secrets)\s*\(\s*[^,]+,\s*[^)]+\)' -C2Length of output: 9126
Add optional errors parameter shim to all verify_* functions
The unit tests in state-manager/tests/unit/tasks/test_verify_graph.py still call the legacy signatures—passing in an explicit errors list—so we need to update each verify_* function in state-manager/app/tasks/verify_graph.py to accept an optional errors parameter and mutate it (while still returning the list for the new style). Please add a short deprecation note (and/or a warnings.warn(..., DeprecationWarning)) and remove this shim once all tests are migrated.
• Functions to update (signatures currently lack an errors param):
verify_nodes_names(nodes: list[NodeTemplate])verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str)verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode])verify_node_identifiers(nodes: list[NodeTemplate])verify_secrets(graph_template: GraphTemplate, database_nodes: list[RegisteredNode])
• Tests invoking legacy style (examples):
await verify_nodes_names(nodes, errors)at lines 34, 47, 88await verify_nodes_namespace(nodes, "test", errors)at lines 106, 119, 134await verify_node_exists(nodes, database_nodes, errors)at lines 164, 183, 204await verify_node_identifiers(nodes, errors)at lines 223, 236, 283, 297await verify_secrets(graph_template, database_nodes, errors)at lines 323, 341, 360, 381
• Example diff for verify_nodes_names:
--- a/state-manager/app/tasks/verify_graph.py
+++ b/state-manager/app/tasks/verify_graph.py
@@ -12,7 +12,10 @@
async def verify_nodes_names(nodes: list[NodeTemplate]) -> list[str]:
- errors = []
+async def verify_nodes_names(
+ nodes: list[NodeTemplate],
+ errors: list[str] | None = None, # DEPRECATED: remove after tests migrate
+) -> list[str]:
+ if errors is None:
+ errors = []
for node in nodes:
if node.node_name is None or node.node_name == "":
errors.append(f"Node {node.identifier} has no name")
return errorsApply the same pattern (optional errors defaulting to None, in-place mutation, return) to the other four functions.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In state-manager/app/tasks/verify_graph.py around lines 12 to 17, the function
verify_nodes_names currently accepts only nodes and returns a new errors list;
update its signature to accept an optional errors: Optional[list[str]] = None
(and add "import warnings" at the top if missing), then if errors is None set
errors = [] else call warnings.warn("legacy errors parameter is deprecated",
DeprecationWarning) so the function mutates the provided list in-place while
still returning it; apply the same pattern (optional errors param defaulting to
None, deprecation warning when provided, in-place appends, and returning errors)
to verify_nodes_namespace, verify_node_exists, verify_node_identifiers, and
verify_secrets.
| async def verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str) -> list[str]: | ||
| errors = [] | ||
| for node in nodes: | ||
| if node.namespace != graph_namespace and node.namespace != "exospherehost": | ||
| errors.append(f"Node {node.identifier} has invalid namespace '{node.namespace}'. Must match graph namespace '{graph_namespace}' or use universal namespace 'exospherehost'") | ||
| return errors | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Namespace check: add optional errors param for test compatibility
Mirror the compatibility shim and keep return-based semantics.
-async def verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str) -> list[str]:
- errors = []
+async def verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str, errors: list[str] | None = None) -> list[str]:
+ errs: list[str] = []
for node in nodes:
if node.namespace != graph_namespace and node.namespace != "exospherehost":
- errors.append(f"Node {node.identifier} has invalid namespace '{node.namespace}'. Must match graph namespace '{graph_namespace}' or use universal namespace 'exospherehost'")
- return errors
+ errs.append(
+ f"Node {node.identifier} has invalid namespace '{node.namespace}'. "
+ f"Must match graph namespace '{graph_namespace}' or use universal namespace 'exospherehost'"
+ )
+ if errors is not None:
+ errors.extend(errs)
+ return errsOptional clean-up (outside this hunk): define a constant to avoid string duplication.
# near the imports
UNIVERSAL_NAMESPACE = "exospherehost"🤖 Prompt for AI Agents
In state-manager/app/tasks/verify_graph.py around lines 19 to 25, the function
verify_nodes_namespace should accept an optional errors parameter for test
compatibility while retaining return-based semantics; update the signature to
accept errors: Optional[list[str]] = None, initialize it to a new list if None,
append namespace validation messages to that list, and still return the list at
the end. Also replace the literal "exospherehost" with a UNIVERSAL_NAMESPACE
constant (define it near imports) to avoid string duplication.
| async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode]) -> list[str]: | ||
| errors = [] | ||
| template_nodes_set = set([(node.node_name, node.namespace) for node in nodes]) | ||
| database_nodes_set = set([(node.name, node.namespace) for node in database_nodes]) | ||
|
|
||
| nodes_not_found = template_nodes_set - database_nodes_set | ||
|
|
||
| for node in nodes_not_found: | ||
| errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.") | ||
| return errors | ||
|
|
There was a problem hiding this comment.
Existence check: add optional errors param for compatibility
Keep the new return style while supporting old callers.
-async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode]) -> list[str]:
- errors = []
+async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode], errors: list[str] | None = None) -> list[str]:
+ errs: list[str] = []
@@
- for node in nodes_not_found:
- errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.")
- return errors
+ for node in nodes_not_found:
+ errs.append(f"Node {node[0]} in namespace {node[1]} does not exist.")
+ if errors is not None:
+ errors.extend(errs)
+ return errs📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode]) -> list[str]: | |
| errors = [] | |
| template_nodes_set = set([(node.node_name, node.namespace) for node in nodes]) | |
| database_nodes_set = set([(node.name, node.namespace) for node in database_nodes]) | |
| nodes_not_found = template_nodes_set - database_nodes_set | |
| for node in nodes_not_found: | |
| errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.") | |
| return errors | |
| async def verify_node_exists( | |
| nodes: list[NodeTemplate], | |
| database_nodes: list[RegisteredNode], | |
| errors: list[str] | None = None | |
| ) -> list[str]: | |
| errs: list[str] = [] | |
| template_nodes_set = set((node.node_name, node.namespace) for node in nodes) | |
| database_nodes_set = set((node.name, node.namespace) for node in database_nodes) | |
| nodes_not_found = template_nodes_set - database_nodes_set | |
| for node in nodes_not_found: | |
| errs.append(f"Node {node[0]} in namespace {node[1]} does not exist.") | |
| if errors is not None: | |
| errors.extend(errs) | |
| return errs |
🤖 Prompt for AI Agents
In state-manager/app/tasks/verify_graph.py around lines 26 to 36, the function
should accept an optional errors parameter to maintain backwards compatibility;
change the signature to accept errors: Optional[list[str]] = None (import
Optional from typing), initialize local_errors = errors if errors is not None
else [] and append missing node messages to local_errors, then return
local_errors so callers using the old pattern still receive the aggregated list
while new callers get the same returned list.
| basic_verify_tasks = [ | ||
| verify_nodes_names(graph_template.nodes), | ||
| verify_nodes_namespace(graph_template.nodes, graph_template.namespace), | ||
| verify_node_exists(graph_template.nodes, database_nodes), | ||
| verify_node_identifiers(graph_template.nodes), | ||
| verify_secrets(graph_template, database_nodes) | ||
| ] | ||
| errors.extend(await asyncio.gather(*basic_verify_tasks)) | ||
|
|
There was a problem hiding this comment.
Flatten asyncio.gather results instead of extending with a list of lists
errors.extend(await asyncio.gather(...)) appends each returned list as a single element. This inflates errors length even when all lists are empty and will incorrectly skip later checks. Flatten before extending.
Apply this diff:
- errors.extend(await asyncio.gather(*basic_verify_tasks))
+ basic_results = await asyncio.gather(*basic_verify_tasks, return_exceptions=False)
+ for errs in basic_results:
+ if errs:
+ errors.extend(errs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| basic_verify_tasks = [ | |
| verify_nodes_names(graph_template.nodes), | |
| verify_nodes_namespace(graph_template.nodes, graph_template.namespace), | |
| verify_node_exists(graph_template.nodes, database_nodes), | |
| verify_node_identifiers(graph_template.nodes), | |
| verify_secrets(graph_template, database_nodes) | |
| ] | |
| errors.extend(await asyncio.gather(*basic_verify_tasks)) | |
| basic_verify_tasks = [ | |
| verify_nodes_names(graph_template.nodes), | |
| verify_nodes_namespace(graph_template.nodes, graph_template.namespace), | |
| verify_node_exists(graph_template.nodes, database_nodes), | |
| verify_node_identifiers(graph_template.nodes), | |
| verify_secrets(graph_template, database_nodes) | |
| ] | |
| basic_results = await asyncio.gather(*basic_verify_tasks, return_exceptions=False) | |
| for errs in basic_results: | |
| if errs: | |
| errors.extend(errs) |
🤖 Prompt for AI Agents
In state-manager/app/tasks/verify_graph.py around lines 254 to 262, the code
does errors.extend(await asyncio.gather(...)) which appends each returned list
as a single element; change it to await the gather into a variable and then
flatten the list-of-lists before extending errors (e.g., collect results = await
asyncio.gather(...), then extend errors with each inner item by either iterating
results and extending by each sublist or using a list comprehension to flatten:
flattened = [item for sub in results for item in sub];
errors.extend(flattened)).
1 participant
Fixing #183 #229 #251
These changes streamline the error handling process and optimize the verification workflow.