Skip to content

refactor(nodes): invocation registration logic #7826

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged

Conversation

psychedelicious
Copy link
Collaborator

Summary

  • Add separate InvocationRegistry class to handle invocation registration. Previously it was all in BaseInvocation and BaseInvocationOutput. This was a bit awkward as logic for execution was mixed up with registration. The actual logic is the same, but the responsibilities are split up in a more logical way.
  • Simplify the invocation typeadapter caching to use @lru_cache(maxsize=1) instead of what effectively was a bespoke version of the same logic.
  • Update all tests and consumers to use the new methods.
  • Improved type annotations for registration logic.

Related Issues / Discussions

n/a

QA Instructions

App should still work. Custom nodes should still work. Works for me. No issues expected as its pretty straightforward.

Merge Plan

n/a

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

@github-actions github-actions bot added python PRs that change python files invocations PRs that change invocations services PRs that change app services python-tests PRs that change python tests labels Mar 23, 2025
Copy link
Collaborator

@jazzhaiku jazzhaiku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change! The code is clearer with these 2 classes separated.
I've left some suggestions for improvement.

@psychedelicious
Copy link
Collaborator Author

I've left some suggestions for improvement.

Thanks! There's no urgency to this PR - I just remembered I've had this branch in my local clone for a couple weeks and figured I should push it. I'll review the suggestions when I have spare time.

@psychedelicious psychedelicious force-pushed the psyche/refactor/simplify-baseinvocation-typeadapter branch from f7521f5 to 5a6f295 Compare March 31, 2025 07:55
@psychedelicious psychedelicious merged commit 5951334 into main Mar 31, 2025
11 checks passed
@psychedelicious psychedelicious deleted the psyche/refactor/simplify-baseinvocation-typeadapter branch March 31, 2025 08:16
@psychedelicious
Copy link
Collaborator Author

I merged this bc it was a blocker for #7847. Can pick up on more improvements in the future.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
invocations PRs that change invocations python PRs that change python files python-tests PRs that change python tests services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants