feat: implement add_extension in Python binding#596
feat: implement add_extension in Python binding#596anakrish merged 10 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
anakrish
left a comment
There was a problem hiding this comment.
Hey @paulolieuthier, Thanks for the contribution.
This looks good. Some suggestions:
Add more tests. E.g:
- Error propagation: What happens when the Python callable raises an exception?
- Type diversity: Passing/returning dicts, lists, sets, numbers, booleans, None
- Zero-arg extensions
- Duplicate registration
- Wrong arity
- Passing a non-callable object
|
@anakrish can you take another look? |
anakrish
left a comment
There was a problem hiding this comment.
@paulolieuthier I have a few more comments. Also we need to note somewhere the clone semantics. That is if an engine instance is cloned, it will share the same python instance and the builtin invocations themselves will be synchronized.
|
Fixed! |
|
@paulolieuthier Looks very good. Can you also explain the clone semantics in the help for the extension method. /// Note: When the engine is cloned, extensions share the same Python callable |
|
Sorry, I had missed your request to improve the docs. I believe it's ready now. |
anakrish
left a comment
There was a problem hiding this comment.
@paulolieuthier LGTM. Thanks for the contribution!
Can you also share your usecase for this feature?
|
Sure! We are experimenting with Rego as a language for our feature pipeline (for a feature store). As our features are defined in Python, we need good Python support to run tests. On production, the features will be evaluated in a Go server, and Rego's Go API is feature complete. So we figured we could help fill in the gaps of the Python bindings. |
|
BTW, are you planning to release a new version? Or should we just pull from main? |
I will make a release next week once #582 also goes in. The RVM is currently exposed to python; we will likely need to implement extension capability in the VM instances as well. So maybe it might be beneficial to maintaing a global table of extensions as opposed to per Engine/VM instance. |
That's my first PR to the project. Please, tell me if there is a better way to do it.