Skip to content

Conversation

@semoal
Copy link
Contributor

@semoal semoal commented Jun 28, 2023

  • Description: Adding async method for CTransformers
  • Issue: I've found impossible without this code to run Websockets inside a FastAPI micro service and a CTransformers model.
  • Tag maintainer: Not necessary yet, I don't like to mention directly
  • Twitter handle: @_semoal

@vercel
Copy link

vercel bot commented Jun 28, 2023

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 10:22am

@semoal semoal force-pushed the semoal/ctransformers-async branch from fcd4255 to ff13a40 Compare June 28, 2023 08:16
Copy link

@QodoAI-Agent QodoAI-Agent left a comment

Choose a reason for hiding this comment

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

PR Analysis

  • 🎯 Main theme: Adding async method for CTransformers
  • 🔍 Description and title: Yes
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: Yes
  • ⚠️ Unrelated changes: No
  • Minimal and focused: Yes, this PR is small and focuses only on adding async method for CTransformers

PR Feedback

  • 💡 Suggestions: The PR is well-structured and follows best practices. The new feature is well-documented and the added test ensures its functionality. However, the async method _acall is currently private (starts with _). If it's intended to be used by the end-users, it should be a public method.

  • 🌱 Minor suggestions: The PR checklist in the description is incomplete. Please ensure to fill it out completely for better tracking and understanding of the PR.

  • 🤖 Code suggestions:

    • In file langchain/llms/ctransformers.py : The async method _acall is currently private. If it's intended to be used by the end-users, consider renaming it to acall. [important]

Copy link
Contributor

@hwchase17 hwchase17 left a comment

Choose a reason for hiding this comment

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

thanks!

@semoal
Copy link
Contributor Author

semoal commented Jun 29, 2023

Going to fix lint step asap 😅

@semoal semoal force-pushed the semoal/ctransformers-async branch from ff13a40 to acf28e9 Compare June 29, 2023 07:58
@semoal
Copy link
Contributor Author

semoal commented Jun 29, 2023

Fixed @hwchase17 :) thanks to you btw

@semoal semoal force-pushed the semoal/ctransformers-async branch from acf28e9 to ddd4e57 Compare June 29, 2023 10:22
"""
text_callback = None
if run_manager:
text_callback = partial(run_manager.on_llm_new_token, verbose=self.verbose)
Copy link
Contributor

Choose a reason for hiding this comment

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

@agola11 if i understand correctly, we don't need to pass self.verbose in to run manager anymore right? since relevant callback handlers are added during Chain init if verbose is set?

@semoal
Copy link
Contributor Author

semoal commented Jul 9, 2023

Can we merge this?

@semoal semoal requested review from dev2049 and hwchase17 July 9, 2023 19:07
@baskaryan baskaryan merged commit 21a353e into langchain-ai:master Jul 10, 2023
@baskaryan
Copy link
Collaborator

done, thanks @semoal!

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