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.

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

Fixed LangChain Circular Import #9342

Merged
merged 7 commits into from
Jun 25, 2024
Merged

Fixed LangChain Circular Import #9342

merged 7 commits into from
Jun 25, 2024
+4 −1

Conversation

MinuraPunchihewa
Copy link
Contributor

Description

This PR fixes a circular import in the LangChain integration that was making it unusable by moving it inside of the relevant function.

Fixes #9333

Type of change

  • 🐛 Bug fix (non-breaking change which fixes an issue)

Verification Process

To ensure the changes are working as expected:

  • Test Location: Specify the URL or path for testing.
  • Verification Steps: Outline the steps or queries needed to validate the change. Include any data, configurations, or actions required to reproduce or see the new functionality.

Checklist:

  • My code follows the style guidelines(PEP 8) of MindsDB.
  • I have appropriately commented on my code, especially in complex areas.
  • Necessary documentation updates are either made or tracked in issues.
  • Relevant unit and integration tests are updated or added.

@MinuraPunchihewa MinuraPunchihewa force-pushed the bugfix/fix_langchain_circular_dep branch from 52eb4aa to 91d5e26 Compare June 13, 2024 18:20
ZoranPandovski
ZoranPandovski requested changes Jun 14, 2024
View reviewed changes
Copy link
Member

@ZoranPandovski ZoranPandovski left a comment

Choose a reason for hiding this comment

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

As far as I know this was an issue since importing the session every request, it initialize the command executor and takes more then 5s. Is there any other way to solve this>

@MinuraPunchihewa
Copy link
Contributor Author

Hmm.. I did notice that it takes a while for some statements to execute at times. I will keep looking for another way.

tmichaeldb
tmichaeldb reviewed Jun 14, 2024
View reviewed changes
Copy link
Contributor

@tmichaeldb tmichaeldb left a comment

Choose a reason for hiding this comment

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

IMO we should avoid this importing pattern all together because it's unclear how long importing a module may take. If this is causing a circular import, we should fix it in a cleaner way (importing SessionController every request can take up to 5s).

I can look into this some more as well

@MinuraPunchihewa
Copy link
Contributor Author

Hey @ZoranPandovski, @tmichaeldb,
I discovered that this circular import is caused by this line; the IntegrationController loads all handler modules via this function when it is initialized, including of course, the LangChain handler which attempts to import the SessionController again, which causes this issue.

By doing this, I think we can also avoid problem of re-loading the session each time that you guys pointed out previously.

Let me know what you think.

ea-rus
ea-rus approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@ea-rus ea-rus left a comment

Choose a reason for hiding this comment

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

I've tested, it works locally.
@tmichaeldb, do you have issues with it on windows?

@ZoranPandovski ZoranPandovski mentioned this pull request Jun 25, 2024
Merged
@tmichaeldb
Copy link
Contributor

As long as we tested that this does not incur additional latency per request let's go ahead and merge

@ZoranPandovski ZoranPandovski merged commit d77d821 into main Jun 25, 2024
13 checks passed
@ZoranPandovski ZoranPandovski deleted the bugfix/fix_langchain_circular_dep branch June 25, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZoranPandovski ZoranPandovski ZoranPandovski approved these changes

@ea-rus ea-rus ea-rus approved these changes

@tmichaeldb tmichaeldb tmichaeldb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Lanchain integration isn't working due to a circular dependency
4 participants
@MinuraPunchihewa @tmichaeldb @ZoranPandovski @ea-rus