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: Graphs #117

Merged
merged 12 commits into from
Aug 8, 2024
Merged

Feat: Graphs #117

merged 12 commits into from
Aug 8, 2024

Conversation

debkanchan
Copy link
Contributor

@debkanchan debkanchan commented Jul 27, 2024

This pull request is related to:

  • A bug
  • A new feature
  • Documentation
  • Other (please specify)

I have checked the following:

  • I have read and understood the contribution guidelines and the code of conduct;
  • I have added new tests (for bug fixes/features);
  • I have added/updated the documentation (for bug fixes / features).

Description:
This PR enables devs to build directed graphs like LangGraph on top of flows to provide a foundation for building agents with Genkit.

Notable points:

  • This is just a directed graph, not a directed acyclic graph. Letting each flow define which flow is next provides a nicer DX compared to edges and more control.

@debkanchan debkanchan mentioned this pull request Jul 27, 2024
3 tasks
@debkanchan
Copy link
Contributor Author

@davidoort @cabljac does this look good? anything you guys wanna add/modify before I write docs and tests?

@davidoort davidoort requested a review from Dabolus July 29, 2024 07:38
@davidoort
Copy link
Contributor

davidoort commented Jul 29, 2024

I'm afk atm, maybe @Dabolus can have a look? This sounds really sick though! Thanks a ton for the contribution 🙏

@cabljac
Copy link

cabljac commented Jul 29, 2024

maybe a removeNode would be useful.

My DAG stuff i'm using in an existing project, so I may take that code and put it elsewhere :)

@debkanchan
Copy link
Contributor Author

maybe a removeNode would be useful.

I'll add that

@debkanchan
Copy link
Contributor Author

I really want to enable streaming for this. Rn all I can think of a special functions called addTerminalNode and make streaming callback available only to the terminal nodes. Is there any better way? @davidoort @Dabolus @cabljac

@debkanchan
Copy link
Contributor Author

Streaming support and remove node added

@debkanchan
Copy link
Contributor Author

Gonna need an illustration for the readme

@debkanchan debkanchan marked this pull request as ready for review August 1, 2024 15:35
@debkanchan
Copy link
Contributor Author

@davidoort @Dabolus @EPMatt rebiew plis? 🥺

Copy link
Collaborator

@Dabolus Dabolus left a comment

Choose a reason for hiding this comment

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

From a code perspective it looks good to me!
However, I noticed that sometimes the plugin is referred to as genkitx-graph while other times it's referred to as genkitx-graphs (plural). Can you uniform the names please?

@debkanchan
Copy link
Contributor Author

From a code perspective it looks good to me! However, I noticed that sometimes the plugin is referred to as genkitx-graph while other times it's referred to as genkitx-graphs (plural). Can you uniform the names please?

fixed

@debkanchan
Copy link
Contributor Author

@davidoort @EPMatt rebiew plis?

davidoort
davidoort previously approved these changes Aug 8, 2024
Copy link
Contributor

@davidoort davidoort left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long, appreciate the patience and amazing contribution! So stoked to try this!

@davidoort davidoort merged commit 12a0f1b into TheFireCo:main Aug 8, 2024
1 check passed
@EPMatt
Copy link
Collaborator

EPMatt commented Aug 9, 2024

@all-contributors add @debkanchan as a code and docs contributor

Copy link
Contributor

@EPMatt

I've put up a pull request to add @debkanchan! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants