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

Replace async workaround within document loader #1774

Merged

Conversation

TheTechmage
Copy link
Contributor

@TheTechmage TheTechmage commented May 17, 2022

After talking with Timo, the document loader uses a separate thread due to the fact that PyLD is synchronous code and calls
into the document loader (which requires asynchronous code). When swapping out the cache with (for example) a Redis based cache, the exception got Future attached to a different loop may arise due to the current separate thread/event loop based implementation.

Switching to nest_asyncio over using a separate thread/event loop resolves the issues that were observed.

After talking with Timo, the document loader uses a seperate thread due
to the fact that PyLD is synchronous code and calls into the document
loader (which requires asynchronous code). When swapping out the cache
with a Redis based cache, new exceptions rose up due to this
implementation. No matter what I tried, it seemed impossible to keep the
separate thread/asyncio event loop due to the error `got Future attached
to a different loop`.

Switching to `nest_asyncio` over using a separate thread/event loop
resolves the issues that I was observing.

Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
@TheTechmage TheTechmage marked this pull request as ready for review May 17, 2022 16:04
@TheTechmage TheTechmage changed the title Replace async hack within document loader Replace async workaround within document loader May 17, 2022
@dbluhm dbluhm added the 0.7.4 label May 17, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1774 (40e40eb) into main (27b238a) will decrease coverage by 0.00%.
The diff coverage is 23.52%.

@@            Coverage Diff             @@
##             main    #1774      +/-   ##
==========================================
- Coverage   95.23%   95.23%   -0.01%     
==========================================
  Files         525      525              
  Lines       33056    33059       +3     
==========================================
+ Hits        31482    31484       +2     
- Misses       1574     1575       +1     

@ianco ianco requested a review from TimoGlastra May 17, 2022 20:18
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

LGTM! Can't speak however on nest_asyncio, haven't used it before

@ianco ianco enabled auto-merge May 18, 2022 14:14
@ianco ianco merged commit 11dd68f into openwallet-foundation:main May 18, 2022
@TheTechmage TheTechmage deleted the fix/document-loader-thread branch May 18, 2022 14:42
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