feat: use importlib when deserializing callables #8648
Conversation
LastRemote
requested review from
dfokina and
anakin87
and removed request for
a team
github-actions
bot
added
topic:tests
type:documentation
| streaming_callback = getattr(module, function_name, None) | ||
| if not streaming_callback: | ||
| try: | ||
| module = import_module(module_name, None) |
There was a problem hiding this comment.
@LastRemote Thanks for opening this PR! We merged a change related to this part of the code base yesterday. Could you please update it so that we also use a thread safe import here too? 91619a7#diff-3a452f86d110b4f40389e0e4551b372188b5fe158aafe0afd5226d0007820457R139
We'll get back to you with a full review next week!
There was a problem hiding this comment.
@LastRemote Thanks for opening this PR! We merged a change related to this part of the code base yesterday. Could you please update it so that we also use a thread safe import here too? 91619a7#diff-3a452f86d110b4f40389e0e4551b372188b5fe158aafe0afd5226d0007820457R139 We'll get back to you with a full review next week!
Hey, sorry for missing this. Sure, will do.
There was a problem hiding this comment.
@julian-risch Updated!
LastRemote
force-pushed
the
feat/use-importlib-when-deserializing-callable
branch
from
dcecb97 to
03c2b9b
Compare
Pull Request Test Coverage Report for Build 12593551802Details
💛 - Coveralls |
There was a problem hiding this comment.
I took a look and it seems good (I left a minor comment).
@julian-risch feel free to do a final review.
releasenotes/notes/use-importlib-when-deserializing-callable-1f36f07c4518c2cf.yaml
Outdated
Show resolved
Hide resolved
LastRemote
force-pushed
the
feat/use-importlib-when-deserializing-callable
branch
from
03c2b9b to
c339c12
Compare
|
I'm merging, since I need to do further work on the same function, to allow the deserialization of class methods (new feature). |
Related Issues
Proposed Changes:
Supports local package imports via importlib.import_module
How did you test it?
Existing unit tests passed. I also added a new negative test for this.
Notes for the reviewer
This is a rather small change but it is causing issues for me right now. Really hope this can be included in 2.9.0!
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.