Skip to content

Add Flyte Callback Handler #6139

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.

Sign up for GitHub

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

Jump to bottom
Merged
hinthornw merged 25 commits into from
Jun 30, 2023
Merged

Add Flyte Callback Handler #6139

hinthornw merged 25 commits into from
Jun 30, 2023

Conversation

@samhita-alla
Copy link
Contributor

@samhita-alla samhita-alla commented Jun 14, 2023 •
edited
Loading

This PR adds a Flyte callback handler that can be triggered from within a Flyte task to track LangChain experiments.

image

Before submitting

Who can review?

Tag maintainers/contributors who might be interested:

@samhita-alla
Add Flyte Callback Handler
f8c23bb 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
debug
f4b4706 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
debug
87f8e76 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
debug
e2f765c 
Signed-off-by: Samhita Alla <[email protected]>
vowelparrot
vowelparrot reviewed Jun 18, 2023
View reviewed changes
vowelparrot
vowelparrot reviewed Jun 18, 2023
View reviewed changes
vowelparrot
vowelparrot reviewed Jun 18, 2023
View reviewed changes
@vowelparrot
Copy link
Contributor

vowelparrot commented Jun 18, 2023

Good start! had some requests

@samhita-alla
Copy link
Contributor Author

samhita-alla commented Jun 19, 2023

Hi @vowelparrot! Thanks for reviewing the PR. I'm currently testing this PR and it isn't complete yet. I'll make sure to incorporate your suggestions.

@samhita-alla
debug
880edae 
Signed-off-by: Samhita Alla <[email protected]>
@vercel
Copy link

vercel bot commented Jun 19, 2023

@samhita-alla is attempting to deploy a commit to the LangChain Team on Vercel.

A member of the Team first needs to authorize it.

@samhita-alla
debug
e682047 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
debug
5a3497f 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
debug
932eeef 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
multiple decks to one deck
61d87f9 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
multiple decks to one deck
8470e0e 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
change llm end
a1bb8e1 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
add orient='inex'
edf108c 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
add markdown headings
7a3f044 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
add documentation; callback handler code clean-up
be4ea42 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
add image
f3794e5 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla samhita-alla marked this pull request as ready for review June 20, 2023 09:16
zeryx
zeryx reviewed Jun 20, 2023
View reviewed changes
Copy link

@zeryx zeryx left a comment

Choose a reason for hiding this comment

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

pandas and spacy are optional langchain imports, added some comments

langchain/callbacks/flyte_callback.py Outdated
Comment on lines 92 to 93
self.pandas = import_pandas()
spacy = import_spacy()
Copy link

@zeryx zeryx Jun 20, 2023

Choose a reason for hiding this comment

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

Minor consideration, both spacy and pandas are optional imports (as per the poet pyproject.toml.

This may fail as both pandas and spacy are not imported by flyte

https://github.com/hwchase17/langchain/blob/master/pyproject.toml#L88
https://github.com/hwchase17/langchain/blob/master/pyproject.toml#L29

Copy link
Contributor

@vowelparrot vowelparrot Jun 20, 2023

Choose a reason for hiding this comment

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

What are you suggesting here?

Copy link
Contributor Author

@samhita-alla samhita-alla Jun 21, 2023

Choose a reason for hiding this comment

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

@zeryx, please let me know your suggestions.

Copy link
Contributor

@vowelparrot vowelparrot Jun 28, 2023

Choose a reason for hiding this comment

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

It's up to you @samhita-alla but if you still want SOME logging even if users don't want to install pandas and spacy, you could log a warning and then reduce the number of facets logged

Copy link
Contributor Author

@samhita-alla samhita-alla Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@vowelparrot, the callback will now log a warning if spacy is not installed. As for pandas, it is automatically installed with flytekit, so there shouldn't be any issues in that regard.

Let me know if you have any further feedback. The PR can be merged if no further changes are required.

cosmicBboy
cosmicBboy reviewed Jun 20, 2023
View reviewed changes
@vercel
Copy link

vercel bot commented Jun 21, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Jun 29, 2023 8:42am

@samhita-alla
resolve lint errors
2d9c90d 
Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla
Copy link
Contributor Author

samhita-alla commented Jun 22, 2023

@vowelparrot, could you please review the PR again?

@hinthornw hinthornw changed the base branch from master to wfh/flyte-callbcak June 30, 2023 16:53
@hinthornw hinthornw merged commit f3068ed into langchain-ai:wfh/flyte-callbcak Jun 30, 2023
hinthornw added a commit that referenced this pull request Jun 30, 2023
@hinthornw @samhita-alla
Add Flyte Callback Handler (#6139) (#6986)
e462584 
Signed-off-by: Samhita Alla <[email protected]>
Co-authored-by: Samhita Alla <[email protected]>
vowelparrot pushed a commit that referenced this pull request Jul 4, 2023
@hinthornw @samhita-alla
Add Flyte Callback Handler (#6139) (#6986)
314c1ee 
Signed-off-by: Samhita Alla <[email protected]>
Co-authored-by: Samhita Alla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@zeryx zeryx zeryx left review comments

@cosmicBboy cosmicBboy cosmicBboy left review comments

@vowelparrot vowelparrot vowelparrot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vowelparrot vowelparrot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@samhita-alla @vowelparrot @zeryx @cosmicBboy @hinthornw @dev2049