Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: jaqpot-223 #14

Merged
merged 12 commits into from
Sep 10, 2024
Merged

Feat: jaqpot-223 #14

merged 12 commits into from
Sep 10, 2024

Conversation

johnsaveus
Copy link
Contributor

Torch Graph Inference for Binary Classification

Comment on lines 46 to 52
onnx_model = base64.b64decode(request.model["rawModel"])
ort_session = onnxruntime.InferenceSession(onnx_model)
feat_config = request.extraConfig["torchConfig"]["featurizer"]
# Load the featurizer
featurizer = SmilesGraphFeaturizer()
featurizer.load_json_rep(feat_config)
smiles = request.dataset["input"][0]["SMILES"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works for SmilesGraphFeaturizer and smiles single input as an independent feature, but what about the rest of the torch models that users may upload?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SmilesGraphFeaturizer works for all the torch models at the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should rename it to JaqpotGraphFeaturizer. Also we should remove hardcoding the dataset["input"][0]["SMILES"] as this assumes that the model input is always going to be an array with a single smiles inside, but that won't always be the case :/

# Load the featurizer
featurizer = SmilesGraphFeaturizer()
featurizer.load_json_rep(feat_config)
smiles = request.dataset["input"][0]["SMILES"]
Copy link
Member

@alarv alarv Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to hardcode the smiles here, just upload the input to the model

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. Need approval for merge

# Load the featurizer
featurizer = SmilesGraphFeaturizer()
featurizer.load_json_rep(feat_config)
smiles = request.dataset["input"][0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
smiles = request.dataset["input"][0]
def featurize_smiles_array(smiles_array, featurizer):
if not isinstance(smiles_array, list):
raise ValueError("Input must be a list of SMILES strings")
featurized_data = []
for smiles in smiles_array:
try:
features = featurizer.featurize(smiles)
featurized_data.append(features)
except Exception as e:
print(f"Error featurizing SMILES: {smiles}. Error: {str(e)}")
featurized_data.append(None) # or handle the error as appropriate
return featurized_data
# Usage
smiles_array = request.dataset["input"]
data = featurize_smiles_array(smiles_array, featurizer)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is as generic as possible and we don't need to change it in the future. Hardcoding [0] of the input means that if we ever upload 2 smiles inputs this won't work and we won't know why till we find this [0] here 😄

Copy link
Member

@alarv alarv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving so we can merge and we'll fix the comments on another PR

@johnsaveus johnsaveus merged commit a960e5e into main Sep 10, 2024
2 checks passed
@johnsaveus johnsaveus deleted the feat/JAQPOT-223/Inference_for_graphs branch September 10, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants