feat(JAQPOT-238): support not-pickled models #15
Conversation
src/helpers/json_to_predreq.py
Outdated
| featurizer_name = request.model['extraConfig']['featurizers'][0]['name'] | ||
| featurizer_config = request.model['extraConfig']['featurizers'][0]['config'] | ||
| featurizer = recreate_featurizer(featurizer_name, featurizer_config) |
There was a problem hiding this comment.
What if we have more than 1 featurizers?
There was a problem hiding this comment.
On point comment. We should extend this functionality to be able to add as many featurizers as desired, if possible
There was a problem hiding this comment.
@periklis91 is it already supported in jaqpotpy dataset?
There was a problem hiding this comment.
Even if it's not, a for here for a single featurizer would have the same effect as what you're doing, instead of hardcoding the [0]. Then if in the future we support it from jaqpotpy (if it's not already supported) then we don't have to go here and change the [0] to a for.
There was a problem hiding this comment.
@vassilismin I cannot recall atm. Give me a few days to finish my current task and then I will switch to 100% jaqpotpy so I will see it then
There was a problem hiding this comment.
if it already works with featurizers, why not make it already like this? 🤔
There was a problem hiding this comment.
JaqpotpyDataset, as it is right now, should take as featurizer an object of MolecularFeaturizer class. However, featurizers is a list that its unique object is a MolecularFeaturizer. So with featurizers[0] i parse the MolecularFeaturizer to JaqpotpyDataset, not a list. In next steps, we should modify JaqpotpyDataset to take a list of MolecularFeaturizer as input, to support multiple featurizers. Till then we need to parse the first object of the list
There was a problem hiding this comment.
I am on it. I will starting working again on jaqpotpy tomorrow and it will be one of the first things that I will implement
There was a problem hiding this comment.
@periklis91 it's ok. Whenever JaqpotpyDataset is ready for multiple featurizers, we need to do this minor change in inference code
There was a problem hiding this comment.
ok, sorry just to understand! add a # TODO so we don't forget it and leave it like this! Resolving the comment
Added a for loop to recreate all requested featurizers. However, JaqpotpyDatast is not ready for getting a list of multiple featurizers. When we will add this feature in JqpotpyDataset, featurizers[0] will change to featurizers
This pr modifies jaqpotpy-inference to use not-pickled models