move idxml to parquet#44
Merged
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates identification I/O from idXML to idParquet, adds idParquet utilities and notation conversion, adapts AScore/PhosphoRS/LucXor CLIs to DataFrame workflows, unifies phospho-site indexing to 1-based residue positions, and updates tests, docs, and CI for parquet-based inputs/outputs. ChangesidParquet Format Migration and Site Indexing Unification
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 1 |
| Duplication | -4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/run_data_tests.py (1)
56-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlgorithm test node IDs point to removed tests.
At Line 58, the node id references
TestAlgorithmExecution::test_{algorithm}_with_real_data, but that class/test names are no longer present intests/test_data_files.py. This path now fails test discovery.Proposed fix
def run_specific_algorithm_tests(): """Run tests for specific algorithms.""" - algorithms = ["ascore", "phosphors", "lucxor"] - - for algorithm in algorithms: + test_module = Path(__file__).parent / "test_data_files.py" + nodeids = { + "ascore": "TestCLIWithRealData::test_ascore_cli_with_real_data", + "phosphors": "TestCLIWithRealData::test_phosphors_cli_with_real_data", + "lucxor": "TestCLIWithRealData::test_lucxor_cli_with_real_data", + } + + for algorithm, nodeid in nodeids.items(): print(f"\nRunning {algorithm} tests...") try: result = subprocess.run([ sys.executable, "-m", "pytest", - f"test_data_files.py::TestAlgorithmExecution::test_{algorithm}_with_real_data", + f"{test_module}::{nodeid}", "-v" ], capture_output=True, text=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/run_data_tests.py` around lines 56 - 60, The subprocess.run call builds a pytest node id that no longer exists (the f"test_data_files.py::TestAlgorithmExecution::test_{algorithm}_with_real_data" string); update the invocation in the subprocess.run call to use a valid test target — either run the whole test file (e.g. "tests/test_data_files.py") or use a discovery-friendly selector like "-k" with a pattern that matches current test names (e.g. "tests/test_data_files.py", or "tests/test_data_files.py -k real_data" ) so pytest can discover the tests; change the f-string in the subprocess.run args accordingly where subprocess.run is called.onsite/phosphors/cli.py (1)
196-223:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftParallel execution path is still wired to removed idXML objects and will fail for
threads > 1.Line 196 iterates
peptide_ids(undefined in this DataFrame flow), and the collector at Lines 235/237 expectsrows/phospho_countwhile_worker_process_pid_threadedreturnshits. This breaks threaded runs deterministically.Suggested direction
- tasks = [] - for idx, pid in enumerate(peptide_ids): - ... - tasks.append({... old pid payload ...}) + tasks = [] + for idx, (_, group_df) in enumerate(grouped): + tasks.append({ + "idx": idx, + "group_df": group_df, + }) indexed_results = {} with ThreadPoolExecutor(max_workers=workers) as executor: - futures = {executor.submit(_worker_process_pid_threaded, t): t["idx"] for t in tasks} + futures = { + executor.submit( + _process_psm_group, + t["group_df"], + exp, + fragment_mass_tolerance, + fragment_mass_unit, + add_decoys, + logger, + scan_map, + ): t["idx"] + for t in tasks + }Also applies to: 232-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onsite/phosphors/cli.py` around lines 196 - 223, The threaded branch is still iterating over a removed idXML-style variable peptide_ids and building tasks that _worker_process_pid_threaded no longer matches (it returns "rows"/"phospho_count" but the collector expects "hits"), causing failures when workers>1; update the task creation loop to iterate the current DataFrame/rows source (use the actual variable holding peptide rows) and construct each task payload with the same keys the worker expects, or alternatively change the worker to return "hits" to match the collector — specifically align the producer that builds tasks (the block creating tasks, "tasks.append({...})" and the futures mapping) with the consumer/_worker_process_pid_threaded return shape and the collector variables "rows" and "phospho_count" so the keys (e.g., "pid", "hits", "spectrum_reference") and index field ("idx") are consistent across producer, worker, and collector.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@onsite/ascore/cli.py`:
- Around line 195-200: The collector expects worker results to include "rows"
and "phospho_count" but _worker_process_pid_threaded() currently returns
{"status":"success","hits": rows} (and no phospho_count), causing KeyError when
the collector (using indexed_results, grouped, result_rows, stats) reads
res["rows"] and res["phospho_count"]. Fix by making
_worker_process_pid_threaded() return the same keys the collector expects (e.g.
return {"status": "success", "rows": rows, "phospho_count": phospho_count}) or
else update the collector to consume "hits" and compute phospho_count
consistently; apply the same fix to the other threaded collection path
referenced around lines 436-475 so both parallel branches use matching result
keys.
- Around line 396-407: The code is currently writing the original input
peptidoform into the psm_rows dict instead of the localized one; replace the
value used for the "peptidoform" key in the dict appended to psm_rows so it
persists the computed localized peptidoform_out (not str(row.get("peptidoform",
...))). Locate the psm_rows.append block and set "peptidoform": peptidoform_out
(or peptidoform_out or fallback to existing row value if peptidoform_out may be
empty), leaving the "sequence" and other fields unchanged.
In `@onsite/decoy_flr.py`:
- Around line 202-211: parse_tool_idparquet currently assumes the caller passed
the actual idParquet directory (containing psms.parquet); update it to resolve
the same two forms that save_dataframes/load_dataframes accept (a base result
path or the explicit idparquet dir) so decoy_flr can pass the original result
path. Concretely, in parse_tool_idparquet (used by decoy_flr) detect and
normalize the path before opening psms.parquet by reusing the shared resolver
logic from load_dataframes (or implement the same check: if
os.path.isdir(os.path.join(path, "idparquet")) then set path =
os.path.join(path, "idparquet") and then look for psms.parquet), so it works
with both the base result path and the idparquet directory that save_dataframes
writes.
In `@onsite/idparquet.py`:
- Around line 335-358: The current copy-and-positional-replace logic (using
src_psm, tbl, _pa_col, updates and set_column) assumes identical row count and
ordering between the source table and psms_df; add a guard that verifies the
source and output are identical before doing the in-place column swaps: check
that tbl.num_rows == len(psms_df) and that a stable key or ordering column(s)
(e.g., an explicit PSM id or the combination of columns that uniquely identify
rows) match row-by-row between tbl and psms_df; if the check fails, skip the
copy/update path and instead construct a fresh pyarrow Table from psms_df
(preserving types via tbl.schema.field(name) only when safe) or write psms_df
directly to psms.parquet so you do not misalign scores/metadata.
In `@onsite/lucxor/cli.py`:
- Around line 1007-1066: The result row incorrectly references an undefined
peptidoform and preserves the original input peptidoform instead of LucXor's
scored localization: replace the failing "_orig_pf or peptidoform" usage by
persisting the scored winner (seq_str) as the peptidoform (converted to unimod
notation via pyopenms_to_unimod_notation(seq_str) or base_seq fallback) and keep
the original input peptidoform only in the metadata (e.g. preserve _orig_pf in
the "search_engine_sequence" meta entry); update the result_rows entry for
"peptidoform" to use the converted seq_str/base_seq and ensure no undefined
names are referenced.
In `@onsite/onsitec.py`:
- Around line 212-215: The code builds a_map/p_map/l_map using DataFrame row
index labels in by_ref but later looks up ai/pi/li as
peptide_identification_index, causing mismatches for multi-hit PSMs; change
by_ref (and the similar map builders at the other spots) to key the returned
dict by df["peptide_identification_index"] (for rows where hit_index == 0 and
spectrum_reference present) so the map keys match the lookup values used later
(where ai/pi/li are treated as peptide_identification_index).
- Line 376: The merged output is being saved with save_dataframes(output_file,
out_df, proteins_df, template_df=lucxor_df) but the LucXor metadata key
source_idparquet isn't propagated, so downstream files are rebuilt with
defaults; update the save path to copy the source_idparquet metadata from
lucxor_df into the merged output (either by setting
out_df.attrs['source_idparquet'] = lucxor_df.attrs['source_idparquet'] before
calling save_dataframes, or by extending save_dataframes to accept a template
metadata argument and write source_idparquet into the saved file), ensuring the
unique symbol save_dataframes and the template_df=lucxor_df usage are the places
you make the change.
In `@onsite/phosphors/cli.py`:
- Around line 262-265: The current call to save_identifications in the block
that handles result_rows unconditionally passes source_idparquet=id_file which
triggers schema-preserving replacement against the original parquet and can fail
when out_df has fewer rows because some groups returned {"status": "error"};
update the branch in onsite/phosphors/cli.py so that before calling
save_identifications you only pass source_idparquet=id_file when out_df.shape[0]
equals the original input row count (or otherwise when no error groups
occurred), otherwise call save_identifications without the source_idparquet
argument (or use a non-schema-preserving save path); specifically adjust the
logic around the variables out_df, proteins_df, psms_df, save_identifications,
and id_file to conditionally include source_idparquet.
- Around line 686-706: The PSM entry is emitting the original raw_seq instead of
the localized best-isomer (new_sequence); update the psm_rows construction so
that "peptidoform" uses the localized new_sequence (and, if new_sequence is a
sequence object rather than a plain string, use its unmodified string form like
new_sequence.toUnmodifiedString()) instead of raw_seq, and ensure "sequence" is
consistent with the localized peptidoform (use seq or new_sequence
appropriately) so the PhosphoRS localization is reflected in the emitted row.
In `@tests/conftest.py`:
- Around line 75-78: The collection-time skip should also verify the actual
psms.parquet file and mark any test that depends on data fixtures, not just
those with the "data" keyword: update the existence check to require
idparquet_dir.joinpath("psms.parquet") (or equivalent) and, when missing,
iterate items and call item.add_marker(pytest.mark.skip(...)) for tests that
either have "data" in item.keywords OR reference the relevant fixtures in
item.fixturenames (e.g., data_dir, mzml_file, idparquet_dir, psms/parquet
fixture names) so fixture-using tests are skipped at collection time.
In `@tests/test_algorithm_comparison.py`:
- Around line 52-55: The test currently only checks for Luciphor metas inside an
if-block that skips assertions when psm_metavalues is missing or the type is
wrong; make the checks explicit and fail the test when psm_metavalues is absent
or mis-typed by asserting mv is not None and isinstance(mv, np.ndarray) before
extracting names, then assert the names list contains the expected "Luciphor"
entry; apply the same change for the other two occurrences (the blocks around
lines referencing psm_metavalues at the other checks) so the assertions cannot
silently pass.
In `@tests/test_lucxor_regression.py`:
- Around line 467-497: The test test_lucxor_deterministic_with_seed is
misleading because no seed is passed to the CLI and the comparison uses unsorted
score arrays (scores1/scores2), so make the test seed-explicit and order-stable:
add a deterministic seed flag to args_base (e.g., pass a --seed value to the
"lucxor" invocation used by runner.invoke) and, before asserting equality,
stabilize the order by pairing/sorting by a stable key (PSM id) or sorting the
score arrays themselves so scores1 and scores2 are compared in identical order;
update both test_lucxor_deterministic_with_seed and the similar block at lines
514-535 to use the same changes.
---
Outside diff comments:
In `@onsite/phosphors/cli.py`:
- Around line 196-223: The threaded branch is still iterating over a removed
idXML-style variable peptide_ids and building tasks that
_worker_process_pid_threaded no longer matches (it returns
"rows"/"phospho_count" but the collector expects "hits"), causing failures when
workers>1; update the task creation loop to iterate the current DataFrame/rows
source (use the actual variable holding peptide rows) and construct each task
payload with the same keys the worker expects, or alternatively change the
worker to return "hits" to match the collector — specifically align the producer
that builds tasks (the block creating tasks, "tasks.append({...})" and the
futures mapping) with the consumer/_worker_process_pid_threaded return shape and
the collector variables "rows" and "phospho_count" so the keys (e.g., "pid",
"hits", "spectrum_reference") and index field ("idx") are consistent across
producer, worker, and collector.
In `@tests/run_data_tests.py`:
- Around line 56-60: The subprocess.run call builds a pytest node id that no
longer exists (the
f"test_data_files.py::TestAlgorithmExecution::test_{algorithm}_with_real_data"
string); update the invocation in the subprocess.run call to use a valid test
target — either run the whole test file (e.g. "tests/test_data_files.py") or use
a discovery-friendly selector like "-k" with a pattern that matches current test
names (e.g. "tests/test_data_files.py", or "tests/test_data_files.py -k
real_data" ) so pytest can discover the tests; change the f-string in the
subprocess.run args accordingly where subprocess.run is called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c84b1d27-11e2-4c0a-a0ef-4162225da733
⛔ Files ignored due to path filters (9)
data/1.zipis excluded by!**/*.zipdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep1_comet_perc.idparquet/protein_groups.parquetis excluded by!**/*.parquetdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep1_comet_perc.idparquet/proteins.parquetis excluded by!**/*.parquetdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep1_comet_perc.idparquet/psms.parquetis excluded by!**/*.parquetdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep1_comet_perc.idparquet/search_params.parquetis excluded by!**/*.parquetdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep2_comet_perc.idparquet/protein_groups.parquetis excluded by!**/*.parquetdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep2_comet_perc.idparquet/proteins.parquetis excluded by!**/*.parquetdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep2_comet_perc.idparquet/psms.parquetis excluded by!**/*.parquetdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep2_comet_perc.idparquet/search_params.parquetis excluded by!**/*.parquet
📒 Files selected for processing (34)
data/1.z01data/1.z02data/1.z03data/1_ascore_result.idXMLdata/1_consensus_fdr_filter_pep.idXMLdata/1_lucxor_result.idXMLdata/1_phosphors_result.idXMLdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep1.mzMLdata/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep2.mzMLonsite/__init__.pyonsite/ascore/ascore.pyonsite/ascore/cli.pyonsite/decoy_flr.pyonsite/idparquet.pyonsite/lucxor/cli.pyonsite/lucxor/core.pyonsite/lucxor/mass_provider.pyonsite/lucxor/peptide.pyonsite/lucxor/psm.pyonsite/onsitec.pyonsite/phosphors/cli.pyonsite/phosphors/phosphors.pypyproject.tomltests/conftest.pytests/run_data_tests.pytests/test_algorithm_comparison.pytests/test_cli.pytests/test_data_files.pytests/test_decoy_flr.pytests/test_imports.pytests/test_lucxor_regression.pytests/test_onsitec_merge.pytests/test_output_validation.pytests/test_phosphors.py
💤 Files with no reviewable changes (1)
- onsite/ascore/ascore.py
Comment on lines
+202
to
+211
| def parse_tool_idparquet(path: str, tool: str) -> List[PSMRecord]: | ||
| """ | ||
| Parse a tool's idParquet directory into one PSMRecord per identification. | ||
| """ | ||
| score_meta = TOOL_SCORE_META[tool] | ||
| tree = ET.parse(path) | ||
| root = tree.getroot() | ||
| psm_path = os.path.join(path, "psms.parquet") | ||
| if not os.path.isfile(psm_path): | ||
| raise FileNotFoundError(f"Expected psms.parquet in {path}") | ||
|
|
||
| df = pd.read_parquet(psm_path) |
Contributor
There was a problem hiding this comment.
Reuse the shared idParquet path resolver here.
save_dataframes() accepts result and writes result.idparquet, while load_dataframes() resolves both forms. This parser only accepts a literal directory that already contains psms.parquet, so passing the same base path value back into decoy_flr will fail even though the rest of the pipeline accepts it.
Suggested fix
def parse_tool_idparquet(path: str, tool: str) -> List[PSMRecord]:
"""
Parse a tool's idParquet directory into one PSMRecord per identification.
"""
+ from onsite.idparquet import resolve_parquet_path
+
score_meta = TOOL_SCORE_META[tool]
- psm_path = os.path.join(path, "psms.parquet")
+ path, psm_path = resolve_parquet_path(path)
if not os.path.isfile(psm_path):
raise FileNotFoundError(f"Expected psms.parquet in {path}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@onsite/decoy_flr.py` around lines 202 - 211, parse_tool_idparquet currently
assumes the caller passed the actual idParquet directory (containing
psms.parquet); update it to resolve the same two forms that
save_dataframes/load_dataframes accept (a base result path or the explicit
idparquet dir) so decoy_flr can pass the original result path. Concretely, in
parse_tool_idparquet (used by decoy_flr) detect and normalize the path before
opening psms.parquet by reusing the shared resolver logic from load_dataframes
(or implement the same check: if os.path.isdir(os.path.join(path, "idparquet"))
then set path = os.path.join(path, "idparquet") and then look for psms.parquet),
so it works with both the base result path and the idparquet directory that
save_dataframes writes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/python-app.yml:
- Around line 22-26: Replace mutable action tags with full commit SHAs and
disable credential persistence on checkout: update the uses entries such as
"actions/checkout@v4" and "actions/setup-python@v6" to their respective
full-length commit SHAs, and add "persist-credentials: false" to the checkout
step (the step that uses actions/checkout). Apply the same SHA-pinning and
checkout persist-credentials change to the other occurrences of actions/checkout
and any other mutable uses mentioned in the review so all uses: lines are pinned
to commit SHAs and checkout steps include persist-credentials: false.
- Line 158: The shell fallback is inverted in the command string "ls -lh
data/SF_200217_pPeptideLibrary_pool1_HCDnlETcaD_OT_rep1_comet_perc.idparquet ||
echo \"Input idparquet directory found\"": change the fallback to report missing
and fail the step so missing fixtures don't get hidden—e.g. replace the current
fallback with a message like "Input idparquet directory NOT found" and exit
non‑zero (or invert logic to use && echo "found" || { echo "NOT found"; exit 1;
}) so the workflow step fails when the path is absent.
In `@README.md`:
- Around line 167-168: The README option tables incorrectly say "file" for `-id`
and `-out`; update the descriptions to read "idparquet directory" (or "Output
idparquet directory with scores" for `-out`) wherever those options appear (all
three tables/occurrences), ensuring both `-id` and `-out` entries use
"directory" wording consistently.
In `@requirements.txt`:
- Line 5: requirements.txt currently has an unpinned "pyarrow" entry; pin it
with a minimum compatible version and make the same change in pyproject.toml to
keep manifests in sync. Update the dependency to "pyarrow>=X.Y.Z" (choose the
minimal version that supports pyarrow.parquet.read_table,
pyarrow.Table.set_column, and pandas.DataFrame.to_parquet) so
onsite/idparquet.py's uses of pyarrow.parquet.read_table, Table.set_column and
DataFrame.to_parquet are supported consistently across environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4574c481-37ad-4628-95f9-ecf4ed201036
📒 Files selected for processing (19)
.codacy.yml.github/workflows/python-app.ymlREADME.mddocs/README.mddocs/algorithms/ascore.mddocs/algorithms/lucxor.mddocs/algorithms/phosphors.mddocs/benchmark.mdonsite/ascore/cli.pyonsite/lucxor/cli.pyonsite/phosphors/cli.pypyproject.tomlrequirements.txttests/conftest.pytests/run_data_tests.pytests/test_algorithm_comparison.pytests/test_data_files.pytests/test_lucxor_regression.pytests/test_output_validation.py
✅ Files skipped from review due to trivial changes (5)
- .codacy.yml
- docs/benchmark.md
- docs/algorithms/lucxor.md
- docs/algorithms/phosphors.md
- docs/README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- pyproject.toml
- tests/conftest.py
- tests/test_lucxor_regression.py
- tests/test_algorithm_comparison.py
- tests/test_output_validation.py
- tests/test_data_files.py
- onsite/ascore/cli.py
- onsite/phosphors/cli.py
- onsite/lucxor/cli.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onsite/phosphors/cli.py (1)
199-242:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftThe threaded branch's task/result contract is broken.
tasks.append()writesmz/rt/spectrum_reference/hitsat the top level, but_worker_process_pid_threaded()still readstask["pid"]. Even after fixing that, the worker returns{"hits": ...}while the main thread consumesres["rows"]andres["phospho_count"]. With--threads > 1, this path can only return errors or crash during aggregation. Please align one shared contract for the worker input and output before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onsite/phosphors/cli.py` around lines 199 - 242, The threaded task/result contract is inconsistent: tasks currently include keys mz/rt/spectrum_reference/hits but _worker_process_pid_threaded still expects task["pid"] and returns {"hits":...} while the caller expects res["rows"] and res["phospho_count"]. Fix by making the two sides agree: either (A) add "pid" to each task in the tasks.append(...) call and change the final aggregation to consume whatever the worker returns, or (B) update _worker_process_pid_threaded to read task["mz"], task["rt"], task["spectrum_reference"], task["hits"] and to return a dict shaped like {"status":"success","rows": [...], "phospho_count": N} on success (and {"status":"error","reason": ...} on failure); ensure the caller's aggregation loop (expecting res["rows"] and res["phospho_count"]) matches the worker return shape.
♻️ Duplicate comments (1)
onsite/idparquet.py (1)
340-386:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the schema-preserving overwrite behind a row-alignment check.
This path still copies the source parquet and swaps columns positionally. When a caller drops or reorders PSMs — for example,
onsite/phosphors/cli.pycan skip entire groups on error before saving — this either fails on array length mismatch or writes the new scores/metas onto the wrong preserved rows. Fall back to the fresh-write path unless row count and stable keys match row-for-row.Suggested safeguard
if src_psm and os.path.isfile(src_psm): import shutil shutil.copy2(src_psm, os.path.join(out_dir, "psms.parquet")) tbl = pq.read_table(os.path.join(out_dir, "psms.parquet")) + key_cols = [ + c for c in ("peptide_identification_index", "hit_index", "spectrum_reference") + if c in psms_df.columns and c in tbl.schema.names + ] + aligned = tbl.num_rows == len(psms_df) + if aligned and key_cols: + aligned = ( + tbl.select(key_cols).to_pandas().reset_index(drop=True) + .equals(psms_df[key_cols].reset_index(drop=True)) + ) + if not aligned: + df = _fill_schema(psms_df, template=template_df) + if "run_identifier" in df.columns: + df["run_identifier"] = df["run_identifier"].fillna(run_identifier or "") + df.to_parquet(os.path.join(out_dir, "psms.parquet"), index=False) + print(f"Saved {len(df)} PSMs to psms.parquet") + else: src_cols = set(tbl.schema.names) ... - pq.write_table(tbl, os.path.join(out_dir, "psms.parquet")) - print(f"Saved {len(psms_df)} PSMs to psms.parquet") + pq.write_table(tbl, os.path.join(out_dir, "psms.parquet")) + print(f"Saved {len(psms_df)} PSMs to psms.parquet")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onsite/idparquet.py` around lines 340 - 386, Before doing the schema-preserving column swap into the copied src_psm table, verify that the source table and the new psms_df align row-for-row: check that tbl.num_rows == len(psms_df) and that a set of stable key columns (e.g., the PSM stable identifier(s) used by this codebase — compare tbl column(s) vs psms_df column(s) value-by-value) match in order; only then proceed with creating updates, calling _pa_col, setting columns on tbl and writing psms.parquet. If the counts or stable-key ordering do not match, skip the copy/overwrite branch and fall back to the existing “fresh-write” path (i.e., write the new psms_df-derived table instead of swapping columns into the copied src_psm) to avoid length mismatches or misaligned rows; update the logic around src_psm / tbl / updates / pq.write_table accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@onsite/phosphors/cli.py`:
- Around line 668-672: The code mutates shared dicts because it uses
_EMPTY_PHOSPHORS_METAS.copy() which shallow-copies the list but leaves the inner
dicts shared; change construction of metas before calling _make_phosphors_row so
each row gets fresh dicts (e.g., deep-copy or recreate each dict element) and
then set metas[0]["value"]=seq_str (and similarly in the other block around the
_make_phosphors_row call at the 683-687 region) to avoid overwriting previously
stored search_engine_sequence values.
---
Outside diff comments:
In `@onsite/phosphors/cli.py`:
- Around line 199-242: The threaded task/result contract is inconsistent: tasks
currently include keys mz/rt/spectrum_reference/hits but
_worker_process_pid_threaded still expects task["pid"] and returns {"hits":...}
while the caller expects res["rows"] and res["phospho_count"]. Fix by making the
two sides agree: either (A) add "pid" to each task in the tasks.append(...) call
and change the final aggregation to consume whatever the worker returns, or (B)
update _worker_process_pid_threaded to read task["mz"], task["rt"],
task["spectrum_reference"], task["hits"] and to return a dict shaped like
{"status":"success","rows": [...], "phospho_count": N} on success (and
{"status":"error","reason": ...} on failure); ensure the caller's aggregation
loop (expecting res["rows"] and res["phospho_count"]) matches the worker return
shape.
---
Duplicate comments:
In `@onsite/idparquet.py`:
- Around line 340-386: Before doing the schema-preserving column swap into the
copied src_psm table, verify that the source table and the new psms_df align
row-for-row: check that tbl.num_rows == len(psms_df) and that a set of stable
key columns (e.g., the PSM stable identifier(s) used by this codebase — compare
tbl column(s) vs psms_df column(s) value-by-value) match in order; only then
proceed with creating updates, calling _pa_col, setting columns on tbl and
writing psms.parquet. If the counts or stable-key ordering do not match, skip
the copy/overwrite branch and fall back to the existing “fresh-write” path
(i.e., write the new psms_df-derived table instead of swapping columns into the
copied src_psm) to avoid length mismatches or misaligned rows; update the logic
around src_psm / tbl / updates / pq.write_table accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc522a55-5043-413a-8c82-7560358c07f7
📒 Files selected for processing (6)
.codacy.ymlonsite/ascore/cli.pyonsite/idparquet.pyonsite/lucxor/cli.pyonsite/onsitec.pyonsite/phosphors/cli.py
✅ Files skipped from review due to trivial changes (1)
- .codacy.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- onsite/lucxor/cli.py
- onsite/ascore/cli.py
- onsite/onsitec.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
onsite/phosphors/cli.py (1)
29-30: 💤 Low valueReplace lambda assignments with
deffunctions per PEP 8.Assigning lambdas to names is discouraged; use
deffor named functions.Suggested fix
-_EMPTY_META_VALUES = lambda s: [s, "0", "0", "-1.0", "{}", "{}"] -_EMPTY_META_TYPES = lambda: ["string", "int", "int", "double", "string", "string"] +def _EMPTY_META_VALUES(s): + return [s, "0", "0", "-1.0", "{}", "{}"] + +def _EMPTY_META_TYPES(): + return ["string", "int", "int", "double", "string", "string"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onsite/phosphors/cli.py` around lines 29 - 30, Replace the two lambda assignments with proper named functions: change _EMPTY_META_VALUES from a lambda to a def function that accepts parameter s and returns [s, "0", "0", "-1.0", "{}", "{}"], and change _EMPTY_META_TYPES from a lambda to a def function that takes no arguments and returns ["string", "int", "int", "double", "string", "string"]; update any references to these symbols (_EMPTY_META_VALUES, _EMPTY_META_TYPES) as needed and ensure docstring or brief comment follows PEP 8 naming and style.Source: Linters/SAST tools
onsite/idparquet.py (1)
339-344: 💤 Low valueSilent exception swallowing hides parsing failures.
The bare
except Exception: passhides why site-score parsing failed, making debugging difficult. Consider logging the exception at debug level.Suggested improvement
try: d = ast.literal_eval(m["value"]) ss = {int(k): float(v) for k, v in d.items()} except Exception: - pass + logger.debug(f"Failed to parse site scores from metavalue: {m}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onsite/idparquet.py` around lines 339 - 344, The try/except around ast.literal_eval(m["value"]) silently swallows failures when building ss = {int(k): float(v) ...}, so replace the bare except with an except Exception as e that logs the failure (including m["value"] and exception details) at debug level before continuing; keep the existing control flow (the break) but ensure you call the module/class logger (or logging.getLogger(...)) with the exception info (e.g., logger.debug("failed parsing site-score %r", m["value"], exc_info=True)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@onsite/idparquet.py`:
- Around line 309-312: The code currently calls
pq.read_table(os.path.join(out_dir, "psms.parquet")) unconditionally which
crashes when src_psm is None or the source file wasn't copied; update the logic
around src_psm and pq.read_table so you first check whether the target file
exists (e.g., os.path.isfile(os.path.join(out_dir, "psms.parquet"))) and only
call pq.read_table if it does, otherwise handle the missing file explicitly
(either raise a clear error, return an empty table/object, or skip downstream
processing) — adjust the block that defines src_psm and the subsequent
pq.read_table call so they use the existence check and a clear fallback path.
In `@onsite/lucxor/cli.py`:
- Around line 411-416: Guard against None/NaN before iterating
additional_scores: check the value returned by row.get("additional_scores")
(add_scores) and skip or replace it with an empty list when it's None or not
iterable (e.g., if not add_scores: continue or add_scores = add_scores or [])
before the for ascore loop; keep using ascore.get(...) and hit.setMetaValue(...)
unchanged so the loop only runs when add_scores is a proper iterable of score
dicts.
- Around line 558-566: The code currently passes the result of
lookup.findByScanNumber(scan_num) directly into exp.getSpectrum, but
findByScanNumber returns -1 for unresolved scans; add a guard after computing
index (from lookup.findByScanNumber) to detect index == -1 and skip processing
this PSM (e.g., continue the loop or otherwise omit creating spectrum_dict),
ensuring you do not call exp.getSpectrum(-1); reference pep_id.getMetaValue,
lookup.findByScanNumber, and exp.getSpectrum to locate and update the logic.
In `@onsite/phosphors/cli.py`:
- Around line 414-416: The code calls lookup.findByScanNumber(scan_number) in
both _worker_process_pid_threaded and _process_psm_group and does not handle the
sentinel -1 result; update both sites to check the returned index and avoid
calling exp.getSpectrum(index) when index == -1 by logging or skipping that
PSM/group. Specifically, after calling findByScanNumber(store result in e.g.
index), if index == -1 then call the appropriate logger (or increment a skipped
counter) and continue/return early; otherwise proceed to call
exp.getSpectrum(index). Ensure you modify the lookup.findByScanNumber usage in
both _worker_process_pid_threaded and _process_psm_group to include this guard.
- Around line 333-339: The code crashes because exp.getSpectrum(0) is called
when the experiment has no spectra; update the function that constructs
SpectrumLookup to first check the experiment's spectrum count (e.g.
exp.getNrSpectra() or exp.getSpectra().size()) and if zero return an empty
SpectrumLookup immediately, otherwise proceed to call exp.getSpectrum(0),
getNativeID and then lookup.readSpectra(...). Ensure you reference
SpectrumLookup, exp.getSpectrum, getNativeID, and readSpectra in the change so
the guard is applied before any use of getSpectrum(0).
---
Nitpick comments:
In `@onsite/idparquet.py`:
- Around line 339-344: The try/except around ast.literal_eval(m["value"])
silently swallows failures when building ss = {int(k): float(v) ...}, so replace
the bare except with an except Exception as e that logs the failure (including
m["value"] and exception details) at debug level before continuing; keep the
existing control flow (the break) but ensure you call the module/class logger
(or logging.getLogger(...)) with the exception info (e.g., logger.debug("failed
parsing site-score %r", m["value"], exc_info=True)).
In `@onsite/phosphors/cli.py`:
- Around line 29-30: Replace the two lambda assignments with proper named
functions: change _EMPTY_META_VALUES from a lambda to a def function that
accepts parameter s and returns [s, "0", "0", "-1.0", "{}", "{}"], and change
_EMPTY_META_TYPES from a lambda to a def function that takes no arguments and
returns ["string", "int", "int", "double", "string", "string"]; update any
references to these symbols (_EMPTY_META_VALUES, _EMPTY_META_TYPES) as needed
and ensure docstring or brief comment follows PEP 8 naming and style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f35219be-cb09-47a5-a960-cbe04eaebaeb
📒 Files selected for processing (8)
onsite/__init__.pyonsite/ascore/ascore.pyonsite/ascore/cli.pyonsite/decoy_flr.pyonsite/idparquet.pyonsite/lucxor/cli.pyonsite/onsitec.pyonsite/phosphors/cli.py
💤 Files with no reviewable changes (1)
- onsite/ascore/ascore.py
🚧 Files skipped from review as they are similar to previous changes (3)
- onsite/init.py
- onsite/decoy_flr.py
- onsite/ascore/cli.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
onsite/idparquet.py (2)
260-262:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
source_idparquet=Nonestill crashes the write path.Both helpers call
os.path.join(source_idparquet, ...)as if the source were mandatory, so the publicOptional[str]contract is broken:Noneraises immediately here, and a missing source file then fails oncopy2()before any fresh parquet/default-file fallback can run.Possible guard
- src_psm = os.path.join(source_idparquet, "psms.parquet") - shutil.copy2(src_psm, os.path.join(out_dir, "psms.parquet")) - tbl = pq.read_table(os.path.join(out_dir, "psms.parquet")) + src_psm = os.path.join(source_idparquet, "psms.parquet") if source_idparquet else None + out_psm = os.path.join(out_dir, "psms.parquet") + if not src_psm or not os.path.isfile(src_psm): + psms_df.to_parquet(out_psm, index=False) + print(f"Saved {len(psms_df)} PSMs to psms.parquet") + return + shutil.copy2(src_psm, out_psm) + tbl = pq.read_table(out_psm) ... - src = os.path.join(source_idparquet, f"{name}.parquet") + src = os.path.join(source_idparquet, f"{name}.parquet") if source_idparquet else NoneAlso applies to: 317-318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onsite/idparquet.py` around lines 260 - 262, The code assumes source_idparquet is always a string and calls os.path.join(source_idparquet, ...) which raises when source_idparquet is None; change the logic around src_psm creation and copy (the block using src_psm, shutil.copy2 and pq.read_table) to first check if source_idparquet is truthy and the source file exists before joining and copying; if source_idparquet is None or the file is missing, skip the join/copy and instead create or use the fallback/default parquet in out_dir (so pq.read_table reads the fallback), and apply the same guard to the similar block that uses source_idparquet at the other location (lines with the second os.path.join/copy2 usage).
262-294:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly reuse the source table when the rows are provably identical.
This branch copies
psms.parquetand then swaps a subset of columns positionally. Ifpsms_dfwas regrouped, filtered, or reordered,set_column()either throws on length mismatch or writes the new scores/metas onto the wrong unchanged metadata. Guard this path with a row-count + stable-key check and fall back to writing a fresh parquet file when it fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onsite/idparquet.py` around lines 262 - 294, The current path mutates the read parquet Table (tbl) with tbl.set_column assuming row order/identity matches psms_df; add a guard that verifies the rows are provably identical before reusing tbl: check tbl.num_rows == len(psms_df) and validate a stable key column (e.g., a unique PSM id column present in both the Arrow table and psms_df) by comparing tbl.column(<stable_key>) values to psms_df[<stable_key>] (if no single stable key exists, compare a deterministic hash of identifying columns); only if both checks pass, perform the in-place set_column updates (using _pa_col and updates as shown) and call pq.write_table(tbl, ...); otherwise, construct a fresh Arrow Table from psms_df (using pa.Table.from_pandas or building columns from _pa_col results) and write that to parquet instead of calling set_column.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@onsite/ascore/cli.py`:
- Around line 137-141: The code calls exp.getSpectrum(0) without checking for
empty input, which throws on zero-spectrum mzML files; before creating
SpectrumLookup or calling exp.getSpectrum(0), check the experiment for zero
spectra (e.g., if exp.getNrSpectra() == 0 or equivalent) and handle it by
raising/printing a clear CLI error like "input mzML contains no spectra" and
exiting, then proceed with the existing branch that calls exp.getSpectrum(0) and
lookup.readSpectra(...) using the appropriate regex ("spectrum=(?<SCAN>\\d+)" vs
"scan=(?<SCAN>\\d+)").
In `@onsite/idparquet.py`:
- Around line 81-95: The C-terminal modifications are being lost or shifted
because unimod_to_pyopenms produces terminal “(Mod)” notation but
pyopenms_to_unimod lacks a branch to detect/translate C-terminal mod tokens and
_group_residue_mods only groups residue-localized X[UNIMOD:y]; fix by adding
explicit C-terminal handling in pyopenms_to_unimod (e.g., detect trailing
"-(mod)" and trailing "(mod)" at sequence end and map them back to
"[-UNIMOD:ID]" form using _to_u) and update _group_residue_mods to recognize and
emit a C-terminal modification entry (distinct from residue-localized entries)
so round-trips preserve terminal modifications; reference functions:
unimod_to_pyopenms, pyopenms_to_unimod, and _group_residue_mods.
- Around line 43-50: The lazy initialization in _ensure() is racy because
_load_from_db() mutates self._unimod_to_pyo incrementally; fix by making
initialization atomic: in _load_from_db() (or a new helper) create local dicts
(e.g., local_unimod_to_pyo and local_pyo_to_unimod), iterate ModificationsDB and
call _register_mod-like logic into those locals, then at the end assign
self._unimod_to_pyo = local_unimod_to_pyo (and any reverse map) in one
statement; alternatively protect _ensure()/_load_from_db() with a threading.Lock
to serialize initialization—refer to methods _ensure, _load_from_db,
_register_mod and attribute _unimod_to_pyo when applying the change.
---
Duplicate comments:
In `@onsite/idparquet.py`:
- Around line 260-262: The code assumes source_idparquet is always a string and
calls os.path.join(source_idparquet, ...) which raises when source_idparquet is
None; change the logic around src_psm creation and copy (the block using
src_psm, shutil.copy2 and pq.read_table) to first check if source_idparquet is
truthy and the source file exists before joining and copying; if
source_idparquet is None or the file is missing, skip the join/copy and instead
create or use the fallback/default parquet in out_dir (so pq.read_table reads
the fallback), and apply the same guard to the similar block that uses
source_idparquet at the other location (lines with the second os.path.join/copy2
usage).
- Around line 262-294: The current path mutates the read parquet Table (tbl)
with tbl.set_column assuming row order/identity matches psms_df; add a guard
that verifies the rows are provably identical before reusing tbl: check
tbl.num_rows == len(psms_df) and validate a stable key column (e.g., a unique
PSM id column present in both the Arrow table and psms_df) by comparing
tbl.column(<stable_key>) values to psms_df[<stable_key>] (if no single stable
key exists, compare a deterministic hash of identifying columns); only if both
checks pass, perform the in-place set_column updates (using _pa_col and updates
as shown) and call pq.write_table(tbl, ...); otherwise, construct a fresh Arrow
Table from psms_df (using pa.Table.from_pandas or building columns from _pa_col
results) and write that to parquet instead of calling set_column.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58cc0b55-bd3c-4f5c-9f47-bfa40305938f
📒 Files selected for processing (8)
onsite/ascore/cli.pyonsite/idparquet.pyonsite/lucxor/cli.pyonsite/onsitec.pyonsite/phosphors/cli.pyonsite/phosphors/phosphors.pytests/test_onsitec_merge.pytests/test_score_selection.py
💤 Files with no reviewable changes (1)
- tests/test_score_selection.py
🚧 Files skipped from review as they are similar to previous changes (3)
- onsite/phosphors/cli.py
- onsite/onsitec.py
- onsite/lucxor/cli.py
ypriverol
approved these changes
Jun 10, 2026
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
2 participants
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 CodeRabbit
New Features
Bug Fixes
Updates
Documentation & Tests