Add AsyncMarkItDown as a wrapper #32
Conversation
|
@microsoft-github-policy-service agree |
|
@0xRaduan -- thanks for the PR. @afourney @jackgerrits @0xRaduan, This was users know how to create an asynchronous wrapper, but we avoid the code/test duplication? Please lmk your thoughts. |
|
Hey @gagb - good feedback, I agree. In general, this async code is just a wrapper over sync code [1], so technically you don't need to have a full suite of async tests, as all of the functionality should be tested in normal tests. I would suggest keeping just one async test to verify that the wrapper works, and remove everything else. Initially I've migrated all tests to just verify that it works identically for my own's sake. |
I like your framing. Would love @jackgerrits feedback on the strategy. |
|
@0xRaduan can you fix the precommit and test errors |
0xRaduan
force-pushed
the
add-async-wrapper
branch
from
62cb884 to
39d5a08
Compare
|
@gagb - fyi, updated the code, fixed linter, removed all tests and just kept one async test to make sure wrapper works in async environment. |
| async def __aenter__(self): | ||
| """Async context manager entry.""" | ||
| return self | ||
|
|
||
| async def __aexit__(self, exc_type, exc_val, exc_tb): | ||
| """Async context manager exit.""" | ||
| pass |
There was a problem hiding this comment.
Why bother making this a context manager if it doesn't do anything? Not sure if I'm missing something here though
| func = partial(self._markitdown.convert, file_path, **kwargs) | ||
| return await self._loop.run_in_executor(None, func) |
There was a problem hiding this comment.
Can you confirm that this will run in a non blocking way?
gagb
added
the
awaiting op response
My take on how this code should support async, given all underlying libraries are not supporting async.
For more details/context, see: #13 (comment)