Refactor get_instance functions to not use dispatch #197
Labels
persistence
Secure persistence feature
Comments
BenjaminBossan
added a commit
to BenjaminBossan/skops
that referenced
this issue
Oct 21, 2022
Resolves skops-dev#197 Description Currently, during the dispatch of the get_instance functions, the class stored in the state is being loaded to determine which function to dispatch to. This is bad because module loading can be dangerous. We will add auditing but it is intended to be on the level of get_instance itself, not for the dispatch mechanism. In this PR, the state returned by get_state functions is augmented with the name of the get_instance method required to load the object. This way, we can look up the correct method based on the state and don't need to use the modified singledispatch mechanism, thus avoiding loading modules during dispatching. Implementation Whereas for get_state, we still rely in singledispatch, for get_instance we now use a simple dictionary that looks up the function based on its name (which in turn is stored in the state). The dictionary, going by the name of GET_INSTANCE_MAPPING, is populated similarly to how the get_instance functions were registered previously with singledispatch. There was an issue with circular imports (e.g. get_instance > GET_INSTANCE_MAPPING > ndarray_get_instance > get_instance), hence the get_instance function was moved to its own module, _dispatch.py (other name suggestions welcome). For some types, we now need extra get_state functions because they have specific get_instance methods. So e.g. sgd_loss_get_state just wraps reduce_get_state and adds sgd_loss_get_instance as its loader. Coincidental changes Since we no longer have to inspect the contents of state to determine the function to dispatch to for get_instance, we can fall back to the Python implementation of singledispatch instead of rolling our own. This side effect is a big win. The function Tree_get_instance was renamed to tree_get_instance for consistency. In the debug_dispatch_functions, there was some code from a time when the state was allowed not to be a dict (json-serializable objects). Now we always have a dict, so this dead code was removed. Also, this fixture was elevated to module-level scope, since it only needs to run once.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is more of a reminder for myself after discussion with Adrin.
Currently, during the dispatch of the
get_instancefunctions, the class stored in thestateis being loaded to determine which function to dispatch to. This is bad because module loading can be dangerous. We will add auditing but this is intended to be on the level ofget_instanceitself, not for the dispatch mechanism.The solution proposed by Adrin is to store which
get_instancemethod should be used as part of the state, therefore allowing us to avoid having to figure that out via the dispatcher. As a side effect, this change should (hopefully) allow us to avoid having a customsingledispatchimplementation.The text was updated successfully, but these errors were encountered: