Conversation
error message: 'DataFrame' object has no attribute 'CLEAN_EC_pred'
revert: tqdm progress bars as they are only usable for batches ()pred_frequency). This is little helpful so progress bars were added to my NetSolP fork instead
chore: change regex to not only extract id before '| 'but also blank ' '
Reviewer's GuideRefactors PDB download logic to support both AlphaFoldDB and ESM Metagenomic Atlas IDs, tightens prediction triggering conditions, adds GPU awareness and timing, makes NetSolP ensemble size configurable, propagates device usage through CLEAN EC prediction, and surfaces AlphaFold URLs in the UI/config. Sequence diagram for dual-source PDB download routingsequenceDiagram
actor User
participant Script_get_pdb_af_from_fasta as script
participant AlphaFoldAPI as alphafold
participant ESMAPI as esm
User->>script: run get_pdb_af_from_fasta.py
script->>script: extract_ids(FASTA_FILE)
loop for each identifier
script->>script: download_pdb(identifier)
alt identifier starts with MGYP
script->>script: download_from_esm(identifier)
script->>esm: requests.get(ESM_API.format(identifier))
esm-->>script: PDB text
script->>script: write .pdb file
else other identifier
script->>alphafold: requests.get(ALPHAFOLD_API.format(identifier))
alphafold-->>script: JSON with pdbUrl
script->>alphafold: requests.get(pdbUrl, stream=True)
alphafold-->>script: PDB stream
script->>script: write .pdb file
end
end
Sequence diagram for prediction pipeline with device and column checkssequenceDiagram
participant Predict_run_predictions as run_predictions
participant CLEAN_run as run_clean_inference_with_embeddings
participant NetSolP_run as get_preds
participant Torch as torch_device
run_predictions->>Torch: torch.device("cuda" if torch.cuda.is_available() else "cpu")
Torch-->>run_predictions: device
run_predictions->>run_predictions: check CLEAN_EC_pred, CLEAN_probability columns
alt CLEAN EC not present and config['predictions']['ec'] == True and plm_model == esm1b
run_predictions->>CLEAN_run: run_clean_inference_with_embeddings(..., device=device)
CLEAN_run->>CLEAN_run: torch.load(emb_train_path, map_location=device)
CLEAN_run->>CLEAN_run: CLEAN_model(esm_emb_inference.to(device))
CLEAN_run-->>run_predictions: df_clean
run_predictions->>run_predictions: merge CLEAN_EC_pred, CLEAN_probability
else skip CLEAN
run_predictions->>run_predictions: logging.info("CLEAN predictions ... Skipping prediction.")
end
run_predictions->>run_predictions: check netSolP_solubility, netSolP_usability columns
alt solubility prediction enabled and NetSolP columns not present
run_predictions->>NetSolP_run: get_preds(df, args, pred_frequency=2)
NetSolP_run->>NetSolP_run: get_preds_split(..., "Solubility")
NetSolP_run->>NetSolP_run: get_preds_split(..., "Usability")
NetSolP_run-->>run_predictions: df with NetSolP columns
else skip NetSolP
run_predictions->>run_predictions: logging.info("NetSolP predictions already exist... Skipping prediction.")
end
run_predictions-->>run_predictions: return 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 2 issues, and left some high level feedback:
- The new checks in
run_predictionsfor CLEAN and NetSolP ("..." not in df.columns) change behavior from 'all null' to 'column missing'; if partially-null columns are expected you may want to keep the null-based logic or explicitly handle mixed/null cases. - In
_process_selection, the Alphafold URL logic can raise a KeyError whenxref_brendaorxref_alphafolddbis missing and writes to two different columns depending on conditions; consider guarding column access and consistently using a single output field for the Alphafold URL. - The hard-coded absolute
MODELS_PATHinpredict_solubility.pyreduces portability compared to the previous relative path; consider deriving this from configuration or environment variables instead of a scratch-specific path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new checks in `run_predictions` for CLEAN and NetSolP (`"..." not in df.columns`) change behavior from 'all null' to 'column missing'; if partially-null columns are expected you may want to keep the null-based logic or explicitly handle mixed/null cases.
- In `_process_selection`, the Alphafold URL logic can raise a KeyError when `xref_brenda` or `xref_alphafolddb` is missing and writes to two different columns depending on conditions; consider guarding column access and consistently using a single output field for the Alphafold URL.
- The hard-coded absolute `MODELS_PATH` in `predict_solubility.py` reduces portability compared to the previous relative path; consider deriving this from configuration or environment variables instead of a scratch-specific path.
## Individual Comments
### Comment 1
<location path="selectzyme/backend/predict_solubility.py" line_range="15-17" />
<code_context>
-def get_preds(df, args):
+
+@run_time
+def get_preds(df, args, pred_frequency=2):
alphabet_path = os.path.join(args.MODELS_PATH, "ESM1b_alphabet.pkl") # args.MODELS_PATH
</code_context>
<issue_to_address>
**suggestion:** Using `pred_frequency` in place of fixed 5 splits needs validation and guarding against invalid values.
This change adds useful flexibility, but `pred_frequency` should be validated to avoid runtime issues (e.g., division-by-zero or shape mismatches when 0/negative or not aligned with the number of model splits). Please add a guard (e.g., `assert pred_frequency > 0`) and document the expected range/relationship to the model directory splits to prevent confusing errors or silent quality regressions.
```suggestion
@run_time
def get_preds(df, args, pred_frequency=2):
"""
Run solubility predictions.
Parameters
----------
df : pandas.DataFrame
Input sequences / metadata for prediction.
args : Namespace
Configuration namespace. Must provide MODELS_PATH.
pred_frequency : int, optional
Positive integer controlling how often predictions are made per input batch.
Must be > 0 and should be consistent with the number of model splits
configured in the models directory to avoid shape mismatches or
misaligned prediction batches.
"""
if not isinstance(pred_frequency, int):
raise TypeError(
f"pred_frequency must be an integer, got {type(pred_frequency).__name__}"
)
if pred_frequency <= 0:
raise ValueError(
"pred_frequency must be a positive integer greater than 0 to avoid "
"division-by-zero and invalid batching."
)
alphabet_path = os.path.join(args.MODELS_PATH, "ESM1b_alphabet.pkl") # args.MODELS_PATH
```
</issue_to_address>
### Comment 2
<location path="selectzyme/backend/predict_ec.py" line_range="98" />
<code_context>
+ model_emb_inference = CLEAN_model(esm_emb_inference.to(device))
inference_dist = get_dist_map_test(
- emb_train, model_emb_inference, ec_id_dict_train, id_ec_inference_dummy, "cpu", torch.float32)
+ emb_train, model_emb_inference, ec_id_dict_train, id_ec_inference_dummy, device, torch.float32)
inference_df = pd.DataFrame.from_dict(inference_dist)
max_sep_predictions_dict = get_max_sep_predictions_dict(inference_df, gmm_path)
</code_context>
<issue_to_address>
**issue (bug_risk):** The `device` argument here appears undefined in this scope, which will raise at runtime.
Within `clean_max_sep_predictions`, `device` is neither defined nor passed, yet it’s used for `.to(device)` and forwarded to `get_dist_map_test`, which will cause a `NameError` at runtime. Please either add `device` as a parameter to `clean_max_sep_predictions` or infer it from existing inputs (e.g. `model_emb_inference.device`), and confirm the type matches what `get_dist_map_test` expects (string vs `torch.device`).
</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
+15
to
17
| @run_time | ||
| def get_preds(df, args, pred_frequency=2): | ||
| alphabet_path = os.path.join(args.MODELS_PATH, "ESM1b_alphabet.pkl") # args.MODELS_PATH |
There was a problem hiding this comment.
suggestion: Using pred_frequency in place of fixed 5 splits needs validation and guarding against invalid values.
This change adds useful flexibility, but pred_frequency should be validated to avoid runtime issues (e.g., division-by-zero or shape mismatches when 0/negative or not aligned with the number of model splits). Please add a guard (e.g., assert pred_frequency > 0) and document the expected range/relationship to the model directory splits to prevent confusing errors or silent quality regressions.
Suggested change
| @run_time | |
| def get_preds(df, args, pred_frequency=2): | |
| alphabet_path = os.path.join(args.MODELS_PATH, "ESM1b_alphabet.pkl") # args.MODELS_PATH | |
| @run_time | |
| def get_preds(df, args, pred_frequency=2): | |
| """ | |
| Run solubility predictions. | |
| Parameters | |
| ---------- | |
| df : pandas.DataFrame | |
| Input sequences / metadata for prediction. | |
| args : Namespace | |
| Configuration namespace. Must provide MODELS_PATH. | |
| pred_frequency : int, optional | |
| Positive integer controlling how often predictions are made per input batch. | |
| Must be > 0 and should be consistent with the number of model splits | |
| configured in the models directory to avoid shape mismatches or | |
| misaligned prediction batches. | |
| """ | |
| if not isinstance(pred_frequency, int): | |
| raise TypeError( | |
| f"pred_frequency must be an integer, got {type(pred_frequency).__name__}" | |
| ) | |
| if pred_frequency <= 0: | |
| raise ValueError( | |
| "pred_frequency must be a positive integer greater than 0 to avoid " | |
| "division-by-zero and invalid batching." | |
| ) | |
| alphabet_path = os.path.join(args.MODELS_PATH, "ESM1b_alphabet.pkl") # args.MODELS_PATH |
| model_emb_inference = CLEAN_model(esm_emb_inference.to(device)) | ||
| inference_dist = get_dist_map_test( | ||
| emb_train, model_emb_inference, ec_id_dict_train, id_ec_inference_dummy, "cpu", torch.float32) | ||
| emb_train, model_emb_inference, ec_id_dict_train, id_ec_inference_dummy, device, torch.float32) |
There was a problem hiding this comment.
issue (bug_risk): The device argument here appears undefined in this scope, which will raise at runtime.
Within clean_max_sep_predictions, device is neither defined nor passed, yet it’s used for .to(device) and forwarded to get_dist_map_test, which will cause a NameError at runtime. Please either add device as a parameter to clean_max_sep_predictions or infer it from existing inputs (e.g. model_emb_inference.device), and confirm the type matches what get_dist_map_test expects (string vs torch.device).
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.
@soucery-ai summary
Summary by Sourcery
Support downloading PDB structures from both AlphaFoldDB and ESM Metagenomic Atlas and wire AlphaFold cross-references into the UI and config, while updating prediction logic to be device-aware and more robust to existing columns.
New Features:
Enhancements: