Skip to content
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

feat: better graph validation errors #7614

Merged
merged 8 commits into from
Feb 5, 2025

Conversation

psychedelicious
Copy link
Collaborator

@psychedelicious psychedelicious commented Feb 4, 2025

Summary

  • When validating edges, return the error message instead of just a boolean.
  • When raising InvalidEdgeError, ensure we use that error message.
  • Surface the error message to the user in the UI.
  • Tweak the error toast to be a bit less cryptic.
  • Fix issue with Any type fields not accepting collections
  • Fix longstanding issue with iterate nodes not accepting connections from collect nodes

Error before:
image

Error after:
image

Related Issues / Discussions

Should help in troubleshooting the issue here https://discord.com/channels/1020123559063990373/1083864753543331981/1334888935242858506

Closes #3956

QA Instructions

@skunkworxdark Could you please try reproducing that issue with this PR? Hopefully it's easier to see what's going on.

For this specific issue, might need to set breakpoints for each of the 3 fail case returns in these two methods in graph.py to get the full picture:

  • Graph._is_iterator_connection_valid()
  • Graph._is_collector_connection_valid()

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)

@psychedelicious psychedelicious changed the title Psyche/feat/nodes/better iterator error feat: better graph validation errors Feb 4, 2025
@github-actions github-actions bot added python PRs that change python files services PRs that change app services frontend PRs that change frontend files labels Feb 4, 2025
@skunkworxdark
Copy link
Contributor

The test case from the discord thread was reproduced. Creates a much better-looking error message.
image

However, it is a shame that the output of the Collect node can not go into an Any type or an Iterate node as it just makes for a slightly more complicated workflow by having to have it fed into the correct collection primitive first. I wonder if we could have an Iterate node where you can choose the output type or it dynamically decides what it is on the UI based on what is fed into it. I am sure you have been through this many times so feel free to ignore these ramblings of a slightly mad man.

@psychedelicious psychedelicious force-pushed the psyche/feat/nodes/better-iterator-error branch 2 times, most recently from e8390f8 to 526158a Compare February 4, 2025 23:42
@github-actions github-actions bot added the python-tests PRs that change python tests label Feb 4, 2025
@psychedelicious
Copy link
Collaborator Author

@skunkworxdark Thanks, I've fixed both issues:

  • collect -> metadata item
  • collect -> iterate

For collect -> iterate, we now traverse the graph to validate that the collector's input type matches all the iterate node's output connections.

For example, graphs like this should now work, because the collection types are compatible with all iterator outputs:
image

But graphs like this don't work, because we are feeding an integer to a color field:
image

We expect an error like this:
image

Can you please give this a spin again? @JPPhoto your review is also appreciated here

@skunkworxdark
Copy link
Contributor

My testing for the PR is looking good and works fine for me with collect -> iterate. I also tested it with a few of my nodes that have any as input so collect -> Any is working well as well. Finally, tried it with a few of my Collect-Tools nodes that use a List[Any] as input these also seem to be working well

@psychedelicious psychedelicious force-pushed the psyche/feat/nodes/better-iterator-error branch from cbf7d70 to 7e9f0ad Compare February 5, 2025 20:48
@psychedelicious psychedelicious enabled auto-merge (rebase) February 5, 2025 20:48
@psychedelicious psychedelicious merged commit 3e13249 into main Feb 5, 2025
15 checks passed
@psychedelicious psychedelicious deleted the psyche/feat/nodes/better-iterator-error branch February 5, 2025 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files 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.

[bug]: Collect -> Iterate validation failure
3 participants