Skip to content

snapshot example without using onRequest #107

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

Merged
merged 0 commits into from
May 20, 2025
Merged

Conversation

marcusschiesser
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented May 6, 2025

🦋 Changeset detected

Latest commit: e093495

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@llama-flow/core Patch
@llama-flow/http Patch
@llama-flow/llamaindex Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

pkg-pr-new bot commented May 6, 2025

Open in StackBlitz

npm i https://pkg.pr.new/run-llama/llama-flow/@llama-flow/core@107
npm i https://pkg.pr.new/run-llama/llama-flow/@llama-flow/llamaindex@107

commit: 134b294

Copy link
Contributor

github-actions bot commented May 6, 2025

@himself65 himself65 marked this pull request as ready for review May 13, 2025 03:57
Comment on lines 68 to 72
const snapshot = await getContext().snapshot();
return humanInteractionRequestEvent.with({
reason: askName.function.arguments,
snapshot,
});
Copy link
Member

Choose a reason for hiding this comment

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

good idea, but I'm worry if this will leak the implementation into the workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could create the snapshot automatically for this event type, e.g.:

const humanInteractionRequestEvent = withSnapshot(workflowEvent<{
  reason: string;
}>());

but i think it adds more complexity.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like calling snapshot in the handler inside is a bad idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't see a big problem but you can call it implicitly for an event wrapped with withSnapshot

@himself65 himself65 force-pushed the ms/snapshot-review branch from 134b294 to a897a47 Compare May 20, 2025 02:34
@himself65 himself65 merged commit e093495 into snapshot-2 May 20, 2025
@himself65 himself65 deleted the ms/snapshot-review branch May 20, 2025 02:45
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.

2 participants