Feat/custom nodes support

[srijanpatel]

This pull request introduces several changes to the backend/pyspur module, focusing on enhancing node management, simplifying class structures, and improving code readability. The most important changes include the introduction of a new FunctionToolNode class, updates to the NodeRegistry to use a NodeInfo schema, and various docstring improvements.

Enhancements to Node Management:

• backend/pyspur/nodes/decorator.py: Introduced the FunctionToolNode class to encapsulate function-based tools, handling parameter extraction, template rendering, and function execution.
• backend/pyspur/nodes/registry.py: Updated the NodeRegistry to use a new NodeInfo schema for storing metadata about nodes, improving the organization and retrieval of node information. [1] [2] [3]

Simplification and Refactoring:

• backend/pyspur/nodes/base.py: Simplified the BaseNode class by removing default values for some attributes and adding new optional attributes like subcategory.
• backend/pyspur/nodes/factory.py: Refactored the NodeFactory class to use direct attribute access instead of dictionary keys for node type information, enhancing readability and performance. [1] [2]

Documentation Improvements:

• Updated docstrings in various files to follow a more consistent and concise format, improving code readability and maintainability. [1] [2] [3] [4]

These changes collectively improve the structure, readability, and functionality of the backend/pyspur module, making it easier to manage and extend node-related features.

---

Important

Add FunctionToolNode class and update NodeRegistry to use NodeInfo schema, with refactoring and documentation improvements.

• Node Management:
  • Add FunctionToolNode class in decorator.py for function-based tools, handling parameter extraction, template rendering, and execution.
  • Update NodeRegistry in registry.py to use NodeInfo schema for node metadata.
• Refactoring:
  • Simplify BaseNode in base.py by removing default values and adding subcategory attribute.
  • Refactor NodeFactory in factory.py to use direct attribute access for node type information.
• Documentation:
  • Improve docstrings for consistency and clarity in node_management.py, factory.py, and node_types.py.
• Miscellaneous:
  • Add __init__.py creation in cli/main.py during project initialization.
  • Update email in MessageCard.tsx to support@pyspur.dev and add markdown support for messages.

^{This description was created by for 0b02eed. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 05c0a7b in 2 minutes and 49 seconds

More details

• Looked at 658 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 17 drafted comments based on config settings.

1. backend/pyspur/cli/main.py:69

• Draft comment:
  Good addition of init.py creation. Consider handling potential file permission errors and ensuring idempotency when creating init.py.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

2. backend/pyspur/nodes/base.py:74

• Draft comment:
  The reordering and removal of default values seems intentional. Consider documenting any behavioral changes if relying on defaults.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

3. backend/pyspur/nodes/decorator.py:74

• Draft comment:
  Template rendering in the run method uses jinja2 to process config strings. Ensure that input data is sanitized to mitigate potential injection risks.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
  The comment is suggesting to ensure input data is sanitized to mitigate potential injection risks. This is a general suggestion to ensure security, but it doesn't provide a specific code suggestion or point out a specific issue in the code. It falls under the category of asking the author to ensure something, which is against the rules.

4. backend/pyspur/nodes/decorator.py:292

• Draft comment:
  Using cast(ToolFunction, func) is acceptable, but ensure that all decorated tools conform to the expected protocol to avoid runtime errors.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

5. backend/pyspur/nodes/factory.py:48

• Draft comment:
  Refactored attribute access in node type schema conversion improves readability and performance. Ensure all NodeTypeSchema fields are correctly populated.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

6. backend/pyspur/nodes/registry.py:235

• Draft comment:
  The registration for tool functions sets the class_name using a string formatted as '{func.func_name}.node_class'. Consider using the actual node class's name for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The NodeInfo docstring explicitly states that class_name supports dot notation for nested attributes. The current code uses this feature to reference the node_class that is nested within a tool function. Changing to node_class.name would lose the path information needed to find the class within the tool function. This appears to be a deliberate design choice, not a mistake.
  Could there be a reason why using name directly would be better for tool function nodes specifically? Maybe there's a special case I'm missing.
  No, the dot notation is clearly needed here because tool function nodes are nested within their parent functions, and the loading code needs to know how to find them. The current implementation matches the documented behavior.
  The comment should be deleted. The current implementation correctly uses dot notation as designed and documented in the NodeInfo class.

7. backend/pyspur/schemas/node_type_schemas.py:12

• Draft comment:
  The attribute traversal for node_class is well-implemented. Ensure that nested class names are correctly resolved in all cases.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

8. backend/pyspur/nodes/node_types.py:263

• Draft comment:
  When iterating over decorator‐registered nodes, use attribute access (e.g. node.node_type_name) instead of dictionary indexing (node["node_type_name"]) since NodeInfo is a Pydantic model.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. backend/pyspur/nodes/decorator.py:74

• Draft comment:
  Consider caching the compiled Jinja2 templates if the same configuration strings are processed repeatedly to improve performance.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The suggestion is about a real performance opportunity - creating Jinja2 templates repeatedly could be inefficient if the same strings are used multiple times. However, we don't know if this is actually a performance bottleneck in practice. The templates are only created for string config values, and those may not be that common or performance-critical. The comment is speculative without evidence of actual performance issues.
  The comment identifies a legitimate technical optimization, but we don't know if this would meaningfully improve real-world performance. Template compilation may not be a bottleneck compared to the actual function execution.
  While the suggestion is technically valid, we should wait for evidence that template compilation is actually causing performance issues before adding complexity for optimization.
  Delete the comment since it's a speculative optimization suggestion without evidence of actual performance impact.

10. backend/pyspur/cli/main.py:70

• Draft comment:
  You can simplify file creation by using Path's write_text() method when creating init.py files.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

11. backend/pyspur/nodes/base.py:74

• Draft comment:
  Defaulting 'name: str' to an empty string may hide potential issues. Consider enforcing a non‐empty name when instantiating nodes.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment suggests removing the default empty string to enforce non-empty names. However, the actual enforcement happens in init where name is a required parameter. The class variable default doesn't affect runtime behavior. The comment seems to misunderstand how the class is actually used.
  I could be wrong about the significance of class variable defaults in Python's type system. Maybe there are static type checking implications I'm not considering.
  Even if there are static type checking implications, the actual runtime behavior enforces non-empty names through init, which is the important part for correctness.
  The comment should be deleted because it raises a concern about something that isn't actually an issue - the name parameter is properly enforced through init.

12. backend/pyspur/nodes/registry.py:232

• Draft comment:
  Using f"{func.func_name}.node_class" for the class_name field is unconventional. Consider storing the actual node class name to improve clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment appears to misunderstand the design. The docstring explicitly states that class_name supports dot notation for nested attributes, giving examples like "some_var.some_attr.target_class". The use of f"{func.func_name}.node_class" follows this documented pattern. The dot notation appears to be an intentional feature to support accessing nested attributes, not an unconventional choice.
  Could there be a valid reason to prefer direct class names over dot notation that I'm missing? Maybe there are performance or reliability concerns with attribute access?
  The dot notation is a documented feature of the class_name field, and this usage matches the intended design. Any performance impact would be negligible since this is just metadata storage.
  The comment should be deleted as it suggests changing an intentional design feature that is working as documented.

13. backend/pyspur/cli/main.py:112

• Draft comment:
  Typographical Error: In the print message for step 2, 'protoype' is used instead of 'prototype'. Please fix the spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. backend/pyspur/nodes/base.py:188

• Draft comment:
  Typographical suggestion: The docstring for the 'input' parameter in the call method contains the notation Dict[str<predecessor node name>, NodeOutputModel]. This notation can be confusing and might benefit from a clearer format, such as Dict[str, NodeOutputModel] (with an explanation that the keys represent the predecessor node names) or rewording for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

15. backend/pyspur/nodes/factory.py:15

• Draft comment:
  The class docstring now starts immediately with text ("""Create node instances from a configuration.") instead of having a dedicated summary line followed by details. Consider reformatting it for improved readability and consistency with common docstring conventions (e.g., starting with a one-line summary in the imperative mood and then a blank line before details).
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While the comment is technically correct about Python docstring conventions, this seems like a minor stylistic issue. The new docstring is still clear and readable. The change doesn't affect functionality or code quality in any significant way. According to the rules, we should not make purely informative comments or comments about obvious/unimportant issues.
  The docstring format could affect auto-documentation tools and code consistency. Bad documentation practices could compound over time.
  While documentation consistency is valuable, this is a minor formatting issue that doesn't significantly impact code quality or functionality. The docstring is still clear and serves its purpose.
  Delete this comment as it's a minor stylistic suggestion that doesn't materially impact code quality or functionality.

16. backend/pyspur/nodes/factory.py:68

• Draft comment:
  The method docstring now begins immediately with text ("""Create a node instance from a configuration.") without a clear summary line. It might be clearer and more conventional to start with an imperative summary (e.g., 'Creates a node instance from a configuration.') followed by a blank line if additional details are needed.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The diff shows multiple docstrings being changed to use "Create" instead of "Creates". This seems like an intentional style change. The comment is suggesting to revert this change, which goes against what appears to be a deliberate style decision. Style preferences like this are subjective and should be consistent within a codebase.
  Perhaps there's a project-wide style guide that mandates using "Creates" that I'm not aware of. The original style might have been the standard.
  Even if there was a previous style guide, the changes in this PR suggest an intentional shift to the new style. Style changes should be consistent across the codebase rather than debated in individual PR comments.
  Delete this comment as it appears to contradict an intentional style change and doesn't identify a clear problem that needs fixing.

17. backend/pyspur/nodes/registry.py:1

• Draft comment:
  Typographical error: The header comment on line 1 incorrectly reads "# backend/app/nodes/registry.py". It should be updated to reflect the correct file path, e.g., "# backend/pyspur/nodes/registry.py", to avoid confusion.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_RiB4CwnSTAPUSuKl

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 0b02eed in 1 minute and 5 seconds

More details

• Looked at 37 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. backend/pyspur/nodes/registry.py:16

• Draft comment:
  Consider restoring the detailed docstring for NodeInfo. The previous version explained the purpose and attributes of this schema. This helps maintain clarity and usability of the code.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While documentation is generally good, this seems like a case where the docstring would be redundant. NodeInfo inherits from NodeTypeSchema, which presumably has its own documentation. The only addition is subcategory which is self-documenting. The old docstring would likely duplicate information from the parent class.
  I could be wrong about NodeTypeSchema having good documentation. Also, even with parent class docs, local docs can be valuable for quick reference.
  While local docs can be helpful, in this case the class is very simple - it just inherits and adds one optional field. The cognitive overhead of maintaining potentially redundant docs outweighs the benefit.
  The comment should be removed. The inheritance makes the old docstring redundant, and maintaining duplicate documentation could lead to confusion or drift.

2. backend/pyspur/nodes/registry.py:16

• Draft comment:
  Verify that NodeTypeSchema includes all the fields (node_type_name, module, class_name) originally defined in NodeInfo. This is critical to ensure that node registration metadata is not inadvertently lost.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
  The comment is asking the PR author to verify that a specific schema includes all necessary fields from another structure. This is a request for confirmation, which violates the rule against asking the author to confirm their intention or ensure behavior. However, it does point out a potential issue with missing fields, which could be useful if rephrased as a suggestion or observation rather than a request for verification.

3. backend/pyspur/nodes/registry.py:16

• Draft comment:
  Ensure that NodeTypeSchema (the new base class for NodeInfo) includes all the required fields (node_type_name, module, class_name) that were previously defined in NodeInfo. This change must maintain compatibility with how NodeInfo is instantiated below.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

4. backend/pyspur/nodes/registry.py:16

• Draft comment:
  Consider adding a docstring to NodeInfo to explain its role and note that it extends NodeTypeSchema, as this can improve code clarity and maintainability.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. backend/pyspur/nodes/registry.py:1

• Draft comment:
  The comment on line 1 reads "# backend/app/nodes/registry.py", but the file is located at "backend/pyspur/nodes/registry.py". It would be good to update this to accurately reflect the file's location.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_fDJDaEcmzZhoEPOx

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.