Solubility usability prediction NetSolP - #78
Merged
Merged
Conversation
style: remove terminal printouts op: remove dependencies from project and install NetSolP seperately, as onnx models can not be used from esm1b base. seperated prediction required
WIP: GPU detection for performance optimization needed
error message: pandas.errors.MergeError: Passing 'suffixes' which cause duplicate columns {'CLEAN_probability_x', 'CLEAN_EC_pred_x'} is not allowed.
style: outsource code from app.py
…nal packages works? Note: Deployment yet untested on absence of additional predictors. Tried to import only when called but not sure if this was enough.
Reviewer's GuideRefactors prediction logic into a reusable backend function and adds optional NetSolP-based solubility/usability prediction alongside existing CLEAN EC-number prediction, with corresponding config flags and README setup instructions. Sequence diagram for prediction flow using CLEAN and NetSolPsequenceDiagram
participant App as app_main
participant Backend as run_predictions
participant CLEAN as run_clean_inference_with_embeddings
participant NetSolP as get_preds
App->>Backend: run_predictions(df, X, config, analysis_path)
alt EC_prediction_enabled_and_missing
Backend->>CLEAN: run_clean_inference_with_embeddings(sequence_label_esm_emb_dict, emb_train_path, ec_csv_path, model_ckpt_path, out_csv, gmm, device)
CLEAN-->>Backend: df_clean
Backend->>Backend: df.merge(df_clean[accession, CLEAN_EC_pred, CLEAN_probability])
end
alt Solubility_prediction_enabled_and_missing
Backend->>NetSolP: get_preds(df, args)
NetSolP-->>Backend: df_with_netSolP
end
Backend-->>App: df
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
run_predictions,df_cleanis only defined inside the CLEAN branch but is merged unconditionally afterwards; if EC prediction is disabled orplm_modelis notesm1b, this will raise aNameError—guard the merge or initialisedf_cleanappropriately. - Using
argparse.ArgumentParser().parse_args()insiderun_predictionsto build NetSolPargswill consume/validate the process-wide CLI arguments and can conflict with the main app; consider constructing a simpleargparse.Namespaceor a plain object/dict instead of parsing global CLI args here. - In
predict_solubility.get_preds,alphabet_pathis hard-coded toNetSolP-1.0/PredictionServer/models/and ignoresargs.MODELS_PATH; this makes the setup brittle and inconsistent with the README, so it would be better to derive the path fromargs.MODELS_PATH.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_predictions`, `df_clean` is only defined inside the CLEAN branch but is merged unconditionally afterwards; if EC prediction is disabled or `plm_model` is not `esm1b`, this will raise a `NameError`—guard the merge or initialise `df_clean` appropriately.
- Using `argparse.ArgumentParser().parse_args()` inside `run_predictions` to build NetSolP `args` will consume/validate the process-wide CLI arguments and can conflict with the main app; consider constructing a simple `argparse.Namespace` or a plain object/dict instead of parsing global CLI args here.
- In `predict_solubility.get_preds`, `alphabet_path` is hard-coded to `NetSolP-1.0/PredictionServer/models/` and ignores `args.MODELS_PATH`; this makes the setup brittle and inconsistent with the README, so it would be better to derive the path from `args.MODELS_PATH`.
## Individual Comments
### Comment 1
<location path="selectzyme/backend/predict.py" line_range="11" />
<code_context>
+def run_predictions(df, X, config, analysis_path):
+
+ # predict EC numbers with CLEAN if they dont exist yet
+ if df.CLEAN_EC_pred.isnull().all() and df.CLEAN_probability.isnull().all():
+ if config["project"]["plm"]["plm_model"] == "esm1b" and config["project"]["predictions"]["ec"] == True:
+ from selectzyme.backend.predict_ec import run_clean_inference_with_embeddings
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `df_clean` being undefined when EC prediction is disabled or a different PLM model is used.
`df_clean` is defined only inside `if config["project"]["plm"]["plm_model"] == "esm1b" and config["project"]["predictions"]["ec"] == True:`. When that condition is false, the outer `if` still proceeds to the `merge`, but `df_clean` will be undefined and cause a runtime error.
Consider either moving the `merge` inside the inner `if`, or initializing `df_clean = None` before it and only merging when `df_clean` is not `None`, so the control flow matches when EC predictions are actually produced.
</issue_to_address>
### Comment 2
<location path="selectzyme/backend/predict.py" line_range="35-44" />
<code_context>
+ logging.info("Running NetSolP predictions. This might take a long time.")
+
+ # hard coded configurations to pass to NetSolP package
+ parser = argparse.ArgumentParser()
+ parser.add_argument("--MODEL_TYPE", default="ESM1b")
+ parser.add_argument("--MODELS_PATH",
+ default="/scratch/global_1/fmoorhof/NetSolP/models/") # NetSolP-1.0/PredictionServer/models
+ parser.add_argument("--NUM_THREADS", default=os.cpu_count(), type=int)
+ parser.add_argument(
+ "--PREDICTION_TYPE",
+ default="SU",
+ choices=['S', 'U', 'SU'],
+ type=str,
+ help="Either Solubility(S), Usability(U) or Both"
+ )
+ args = parser.parse_args()
+
+ df = get_preds(df=df, args=args)
</code_context>
<issue_to_address>
**suggestion:** Avoid calling `argparse.ArgumentParser().parse_args()` inside a library/helper function.
Calling `parse_args()` here consumes global CLI flags and can cause unexpected exits or conflicts when this helper is used from other entry points (e.g., other CLIs, web apps, notebooks).
Consider instead:
- Constructing a simple config object in code (e.g., `types.SimpleNamespace` or a dataclass), or
- Accepting an `args`-like parameter in `run_predictions`
so that this function remains free of global CLI side effects.
Suggested implementation:
```python
# predict solubility and usability with NetSolP if they dont exist yet
if (
config["project"]["predictions"]["solubility"] is True
and df.netSolP_solubility.isnull().all()
and df.netSolP_usability.isnull().all()
):
from types import SimpleNamespace
from selectzyme.backend.predict_solubility import get_preds
logging.info("Running NetSolP predictions. This might take a long time.")
# hard coded configurations to pass to NetSolP package
args = SimpleNamespace(
MODEL_TYPE="ESM1b",
MODELS_PATH="/scratch/global_1/fmoorhof/NetSolP/models/", # NetSolP-1.0/PredictionServer/models
NUM_THREADS=os.cpu_count(),
PREDICTION_TYPE="SU", # Either Solubility(S), Usability(U) or Both
)
df = get_preds(df=df, args=args)
return df
```
- Ensure `os` is imported at the top of `selectzyme/backend/predict.py` if it is not already: `import os`.
- If you prefer to avoid importing `SimpleNamespace` inside the function, move `from types import SimpleNamespace` to the top-level imports instead.
- If other callers already construct and pass an `args` object into `get_preds`, you may later want to refactor `get_preds` to accept a more explicit config type (e.g., a dataclass) instead of relying on attribute-based namespaces.
</issue_to_address>
### Comment 3
<location path="selectzyme/backend/predict_solubility.py" line_range="14" />
<code_context>
+
+
+def get_preds(df, args):
+ alphabet_path = os.path.join("NetSolP-1.0/PredictionServer/models/", "ESM1b_alphabet.pkl") # args.MODELS_PATH
+
+ with open(alphabet_path, "rb") as f:
</code_context>
<issue_to_address>
**issue (bug_risk):** Use the configured `MODELS_PATH` instead of a hard-coded NetSolP model directory.
`alphabet_path` is hard-coded and doesn’t use `args.MODELS_PATH`, even though that’s already configured in `__main__`. This couples the code to a specific directory layout and CWD.
Instead, derive the path from the configured models directory, e.g.:
```python
alphabet_path = os.path.join(args.MODELS_PATH, "ESM1b_alphabet.pkl")
```
so the model location is controlled via configuration rather than a fixed relative path.
</issue_to_address>
### Comment 4
<location path="README.md" line_range="40-42" />
<code_context>
-## EC number prediction with CLEAN
+## Additional predictors based on ESM1b
+Note: Deployment yet untested on absence of additional predictors. Tried to import only when called but not sure if this was enough.
+### EC number prediction with CLEAN
+Install CLEAN, download weights and place them into the hard coded locations.
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify and slightly rephrase the note about deployment being untested for better grammar.
Consider rephrasing this note for clarity and grammar, for example:
"Note: Deployment is yet untested in the absence of additional predictors. Imports are deferred until the predictors are called, but it is not yet verified that this is sufficient."
```suggestion
## Additional predictors based on ESM1b
Note: Deployment is yet untested in the absence of additional predictors. Imports are deferred until the predictors are called, but it is not yet verified that this is sufficient.
### EC number prediction with CLEAN
```
</issue_to_address>
### Comment 5
<location path="README.md" line_range="43-48" />
<code_context>
+## Additional predictors based on ESM1b
+Note: Deployment yet untested on absence of additional predictors. Tried to import only when called but not sure if this was enough.
+### EC number prediction with CLEAN
+Install CLEAN, download weights and place them into the hard coded locations.
```
-git clone https://github.com/tttianhao/CLEAN.git
</code_context>
<issue_to_address>
**nitpick (typo):** Use standard hyphenation for "hard-coded" in both installation instructions.
Please change both occurrences (in the CLEAN and NetSolP sections) from "hard coded" to "hard-coded locations" to match standard technical writing conventions.
Suggested implementation:
```
Install CLEAN, download weights and place them into the hard-coded locations.
```
```
Install NetSolP, download weights and place them into the hard-coded locations.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+40
to
+42
| ## Additional predictors based on ESM1b | ||
| Note: Deployment yet untested on absence of additional predictors. Tried to import only when called but not sure if this was enough. | ||
| ### EC number prediction with CLEAN |
There was a problem hiding this comment.
suggestion (typo): Clarify and slightly rephrase the note about deployment being untested for better grammar.
Consider rephrasing this note for clarity and grammar, for example:
"Note: Deployment is yet untested in the absence of additional predictors. Imports are deferred until the predictors are called, but it is not yet verified that this is sufficient."
Suggested change
| ## Additional predictors based on ESM1b | |
| Note: Deployment yet untested on absence of additional predictors. Tried to import only when called but not sure if this was enough. | |
| ### EC number prediction with CLEAN | |
| ## Additional predictors based on ESM1b | |
| Note: Deployment is yet untested in the absence of additional predictors. Imports are deferred until the predictors are called, but it is not yet verified that this is sufficient. | |
| ### EC number prediction with CLEAN |
Comment on lines
+43
to
48
| Install CLEAN, download weights and place them into the hard coded locations. | ||
| ``` | ||
| git clone https://github.com/tttianhao/CLEAN.git | ||
| git clone https://github.com/fmoorhof/CLEAN.git | ||
| cd CLEAN/app | ||
| # get model weights (see below) | ||
| python build.py install # requires activated venv/conda |
There was a problem hiding this comment.
nitpick (typo): Use standard hyphenation for "hard-coded" in both installation instructions.
Please change both occurrences (in the CLEAN and NetSolP sections) from "hard coded" to "hard-coded locations" to match standard technical writing conventions.
Suggested implementation:
Install CLEAN, download weights and place them into the hard-coded locations.
Install NetSolP, download weights and place them into the hard-coded locations.
…ded NetSolP model directory. [sourcery suggestions]
fmoorhof
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce configurable prediction pipeline that augments SelectZyme runs with optional EC number and solubility/usability predictions based on ESM1b embeddings.
New Features:
Enhancements:
Documentation: