Conversation
… vendor fiora under assets; fix ms submodule Made-with: Cursor
Replace the vendored local fioRa runtime with a browser-driven online workflow so the MS skill stays lightweight while still exporting MSP, MGF, and PNG outputs. Made-with: Cursor
Remove duplicated NMRNet sources and demo data from the bundled skill while keeping the runtime dictionary and Uni-Core setup intact, reducing the repository footprint without changing the inference workflow. Made-with: Cursor
Add self-contained 31P and 19F NMR skills that predict shifts from CIF crystal structures using shared NMRNet assets, so the new workflows can be run directly from the skills directory. Made-with: Cursor
Update the skills catalog so the new P and F NMR simulation skills are listed with folder links, descriptions, and completed status. Made-with: Cursor
There was a problem hiding this comment.
Code Review
This pull request introduces two new skills, f-nmr-simulation and p-nmr-simulation, for predicting solid-state 19F and 31P NMR chemical shifts using the NMRNet model. Additionally, the ms-spectra-simulation skill has been refactored to utilize the fioRa online application via Playwright browser automation, replacing the previous local installation. Review feedback suggests optimizing the download of large model weights to avoid potential memory issues and using the csv module for safer payload construction in the mass spectrometry simulation script.
I am having trouble creating individual review comments. Click here to see my feedback.
skills/f-nmr-simulation/f_nmr_simulation.py (57)
Using zf.read(zip_path) loads the entire file into memory. Since the model weights are approximately 560 MB, this can lead to Out-Of-Memory (OOM) errors in environments with limited RAM. It is more efficient to stream the file content directly to disk using shutil.copyfileobj.
import shutil
with zf.open(zip_path) as source, open(local_path, "wb") as target:
shutil.copyfileobj(source, target)
skills/p-nmr-simulation/p_nmr_simulation.py (57)
Using zf.read(zip_path) loads the entire file into memory. Since the model weights are approximately 560 MB, this can lead to Out-Of-Memory (OOM) errors in environments with limited RAM. It is more efficient to stream the file content directly to disk using shutil.copyfileobj.
import shutil
with zf.open(zip_path) as source, open(local_path, "wb") as target:
shutil.copyfileobj(source, target)
skills/ms-spectra-simulation/ms_spectra_simulation.py (70-74)
The CSV payload for the fioRa app is constructed using f-strings without escaping. If args.name or args.smiles contains commas or newlines, the resulting CSV will be malformed. Using the csv module ensures that fields are correctly quoted and escaped.
def build_textbox_payload(args: argparse.Namespace) -> str:
import io
import csv
output = io.StringIO()
writer = csv.writer(output, lineterminator="\n")
writer.writerow(["Name", "SMILES", "Precursor_type", "CE", "Instrument_type"])
writer.writerow([args.name, args.smiles, args.precursor, f"{args.ce:g}", args.instrument])
return output.getvalue()
|
@jiaqingxie 可能需要对应修改一下 skills/README.md,把这两个新的 skill 描述加上,其他没什么问题 |
Summary
Closes: #(issue)
References
If this PR adds a new skill
skills/<skill-name>/SKILL.mdfrontmatter includesnameanddescriptionnamematches folder name and uses kebab-caseskills/README.mdand assigned this skill to the correct categoryskills/README.md(name, description, status, and link if applicable)Chosen category in
skills/README.md:Merge/Separate decision note:
Validation
Impact
Checklist