Standardize dashboard-ready output contract#67
Conversation
bibymaths
added
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b01db87452
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for fpath in [out_dir / 'report.html']: | ||
| logger.info(f"{fpath.as_uri()}") | ||
|
|
||
| populate_standard_subdirs(out_dir) |
There was a problem hiding this comment.
In a normal local kinopt run, organize_output_files(out_dir) has already moved the generated CSV/XLSX/PNG files into protein-specific or General/ subdirectories before this call runs, while populate_standard_subdirs() only iterates the result directory root and skips directories. As a result the new tables/ and plots/ folders remain empty apart from top-level files like report.html, so the dashboard-ready contract is not actually produced; call it before organizing or make it recurse into the organized folders.
Useful? React with 👍 / 👎.
| OUT_DIR = ensure_result_dir(args.outdir)["root"] | ||
| OUT_RESULTS_DIR = OUT_DIR / OUT_RESULTS_DIR.name |
There was a problem hiding this comment.
When --outdir is supplied, rebinding this runner module's OUT_DIR does not affect the default out_dir=OUT_DIR already bound inside protwise.paramest.core.process_gene_wrapper, and the later call omits out_dir. In that scenario the per-gene parameter files and plots are still written under the configured default output directory while the workbook/report are written under the requested run directory, leaving the requested result folder incomplete.
Useful? React with 👍 / 👎.
| import kinopt.optimality.KKT as kkt_module | ||
| import kinopt.fitanalysis.__main__ as fitanalysis_module | ||
| import kinopt.fitanalysis.helpers.postfit as postfit_module | ||
| kkt_module.OUT_FILE = out_file |
There was a problem hiding this comment.
For custom --outdir runs, assigning kkt_module.OUT_FILE here does not change the already-bound default argument of process_excel_results(file_path=OUT_FILE) inside post_optimization_results(). The KKT post-processing therefore reads the original default workbook path instead of the newly written out_file, which can fail on a fresh run directory or analyze stale results from a previous run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e08ad03c84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model.callback(conf=model_conf) | ||
| tfopt.callback(mode=tf_mode, conf=tf_conf, outdir=(outdir / "tfopt" if outdir else None)) | ||
| kinopt.callback(mode=kin_mode, conf=kin_conf, outdir=(outdir / "kinopt" if outdir else None)) | ||
| model.callback(conf=model_conf, outdir=(outdir / "protwise" if outdir else None)) |
There was a problem hiding this comment.
When all runs the local TF/KinOpt stages, those stages now keep tfopt_results.xlsx and kinopt_results.xlsx in their result directories and this commit explicitly removed the old copy into data/. The subsequent model.callback(...) only changes ProtWise's output directory, so protwise.runner still reads its defaults from data/tfopt_results.xlsx and data/kinopt_results.xlsx; on a fresh run these files are missing, and on a dirty workspace it can silently model stale results instead of the just-produced workbooks. Pass the newly produced workbook paths as ProtWise inputs, or keep a handoff copy for the chained workflow.
Useful? React with 👍 / 👎.
| write_command(OUT_DIR) | ||
| write_resolved_config(OUT_DIR, config) | ||
| write_metadata( | ||
| OUT_DIR, | ||
| workflow="protwise.runner", |
There was a problem hiding this comment.
These provenance files are written at the result root, but this runner later calls common.utils.display.organize_output_files([OUT_DIR]), which moves every remaining top-level file into General/. The new dashboard parser only looks for metadata.json, command.txt, and config_resolved.yaml at the root, so normal ProtWise runs will report the standard provenance as missing even though it was created. Write/copy the provenance after organizing, or exclude these contract files from the organizer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af71a2818
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| table_ext = {".csv", ".tsv", ".xlsx", ".xls", ".parquet", ".json", ".npy", ".npz"} | ||
| plot_ext = {".png", ".jpg", ".jpeg", ".svg", ".pdf", ".html"} |
There was a problem hiding this comment.
When runs produce non-CSV table/artifact outputs or PDF reports, this sends .json, .npy, .npz, .parquet, and .pdf files into tables/ or plots/, but discover_result_directory() only scans tables with TABLE_EXTENSIONS (.csv/.tsv/.xlsx/.xls) and plots with PLOT_EXTENSIONS (.png/.jpg/.jpeg/.svg/.html). As a result files such as multistart_params.npz, optimized_entities.json, or S_rates_report.pdf are copied into standard folders but then disappear from the dashboard browser; either route unsupported suffixes to artifacts/ or make the parser/viewers include them.
Useful? React with 👍 / 👎.
| pixi_task="kinopt-local", | ||
| python_module="kinopt.local", | ||
| accepted_arguments=( | ||
| ArgumentSpec("conf", "--conf", "path", "Optional TOML/YAML configuration file."), |
There was a problem hiding this comment.
When a dashboard user supplies the advertised optional config for kinopt-local, the generated command includes --conf, but kinopt.local.config.constants has already loaded _CFG from the repository config.toml at import time and parse_args() only accepts --conf as a compatibility option without reading args.conf. This makes dashboard KinOpt runs silently ignore the selected TOML/YAML file while appearing to use it, so custom bounds/inputs from that file are not applied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c0d76bf86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| EXCLUDED_ZIP_PARTS = {"__pycache__", ".pytest_cache", ".mypy_cache", ".ruff_cache", ".git"} | ||
|
|
||
| UPLOAD_EXTENSIONS = {".csv", ".tsv", ".xlsx", ".yaml", ".yml", ".json", ".txt"} |
There was a problem hiding this comment.
When a dashboard user tries to upload the project’s normal config.toml for any workflow, the uploader and validation reject it because .toml is missing from UPLOAD_EXTENSIONS, even though the workflow config input specs and parse_config_file() explicitly support TOML. This makes the advertised config-file upload path unusable for the default config format unless the user manually types an existing path instead.
Useful? React with 👍 / 👎.
| InputSpec("previous_kinopt", "Previous KinOpt result", "kinopt", "Previous kinopt Excel result.", extensions=(".xlsx",)), | ||
| InputSpec("previous_tfopt", "Previous TFOpt result", "tfopt", "Previous tfopt Excel result.", extensions=(".xlsx",)), |
There was a problem hiding this comment.
If a dashboard user assigns a previous KinOpt or TFOpt result here, arguments_from_input_assignments() maps it to kinopt/tfopt, but those names are absent from accepted_arguments, so build_workflow_command() raises ValueError: Unsupported arguments for networkmodel: kinopt before the run can start. The underlying runner accepts --kinopt and --tfopt, so these mapped inputs need matching ArgumentSpecs.
Useful? React with 👍 / 👎.
| parser = argparse.ArgumentParser( | ||
| description="PhosKinTime - SLSQP mRNA-TF Optimization Problem." | ||
| ) | ||
| parser.add_argument("--conf", default=None, help="Compatibility option; configuration is loaded before argument parsing.") |
There was a problem hiding this comment.
When tfopt-local is launched with a selected --conf file, this parser accepts the flag but never reloads _CFG; _CFG = load("local", "tfopt") was already evaluated at import time from the default repository config. In that scenario dashboard users appear to run with their chosen TFOpt config, but custom inputs/bounds from that file are silently ignored.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa8ba1b37c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ValueError: When inputs are inconsistent or unsupported. | ||
| """ | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--conf", default=None, help="Compatibility option; networkmodel settings are loaded before argument parsing.") |
There was a problem hiding this comment.
When a dashboard/config.cli launch passes a custom --conf, this new argument is accepted but never used; networkmodel.config has already been imported at module load and it always reads config.toml before parse_args() runs, so defaults such as KINASE_NET_FILE, RESULTS_DIR, solver settings, and time grids still come from the repository config unless every value is also supplied as an individual CLI flag. In the custom-config scenario the run appears to honor the selected config file while silently using the default one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3234ecca6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| required_inputs=("protein Excel", "phosphosite Excel", "RNA Excel"), | ||
| input_assignments=( | ||
| InputSpec("config", "Config file", "conf", "Optional ProtWise config file.", extensions=(".toml", ".yaml", ".yml", ".json")), | ||
| InputSpec("protein_file", "Protein file", "input_excel_protein", "Protein input Excel file.", extensions=(".xlsx",)), |
There was a problem hiding this comment.
When a dashboard user assigns the ProtWise protein input, validation only accepts .xlsx, but the runner reads this path with pd.read_csv(config['input_excel_protein']) and the default project config points protein_excel at data/input1.csv. This makes the normal CSV protein input impossible to select from the dashboard, while an .xlsx file that passes validation will fail when the workflow starts; the ProtWise protein InputSpec should match the runner's CSV reader (or the runner should read Excel here).
Useful? React with 👍 / 👎.
| InputSpec("kinase_network", "Kinase network file", "kinase_net", "Kinase network CSV/TSV file.", extensions=(".csv", ".tsv")), | ||
| InputSpec("tf_network", "TF network file", "tf_net", "TF network CSV/TSV file.", extensions=(".csv", ".tsv")), | ||
| InputSpec("protein_file", "Protein/MS file", "ms", "Protein/MS data file.", extensions=(".csv", ".tsv", ".xlsx")), | ||
| InputSpec("rna_file", "RNA/mRNA file", "rna", "RNA data file.", extensions=(".csv", ".tsv", ".xlsx")), | ||
| InputSpec("phosphosite_file", "Phosphosite file", "phospho", "Phosphoproteomics data file.", extensions=(".csv", ".tsv", ".xlsx")), |
There was a problem hiding this comment.
These dashboard input specs accept .tsv and .xlsx files for networkmodel data inputs, but networkmodel.io.load_data uses plain pd.read_csv(...) for the kinase net, TF net, MS, and RNA paths without Excel handling or a tab separator. In the dashboard path, selecting an uploaded TSV/XLSX passes validation and builds the command, then the workflow either fails immediately on Excel or parses TSVs as a single comma-delimited column and fails later with missing columns; restrict these extensions to CSV or add matching readers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9565a25d15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parser = argparse.ArgumentParser( | ||
| description="PhosKinTime - ODE Parameter Estimation of Cell Signalling Events in Temporal Space" | ||
| ) | ||
| parser.add_argument("--conf", default=None, help="Compatibility option; configuration is loaded before argument parsing.") |
There was a problem hiding this comment.
When a dashboard or config.cli model --conf ... launch supplies a ProtWise config, this newly accepted flag is parsed but never used: this module has already imported config.constants from the repository config.toml, and extract_config() only reads the other CLI fields. In that custom-config scenario the run appears to accept the selected config while silently using the default bounds, bootstraps, time grids, and input paths unless each one is also passed as a separate argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5604439fb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _RAW_CFG = tomllib.load(_fh) | ||
| _ODE_BASE = (_RAW_CFG.get("ode", {}) or {}) | ||
| _ODE_MODES = (_ODE_BASE.get("modes", {}) or {}) | ||
| _CFG = {**_ODE_BASE, **(_ODE_MODES.get("local", {}) or {})} |
There was a problem hiding this comment.
When a custom --conf contains a partial nested [ode.modes.local] override, this shallow merge replaces the whole nested table from [ode] instead of preserving unspecified keys, unlike the default load("local", "ode") path. For example, overriding only ode.modes.local.sensitivity.perturbation drops ode.sensitivity.enabled = false and ode.sensitivity.morris, so config.constants falls back to SENSITIVITY_ANALYSIS=True and default Morris settings even though config.config parsed the custom config with a deep merge; modules such as protwise.paramest.core and protwise.sensitivity.analysis then run with different settings from the recorded resolved config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63fb6bb7de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| skip_names = set(STANDARD_SUBDIRS) | {"metadata.json", "command.txt", "console.log", "config_resolved.yaml"} | ||
| table_ext = {".csv", ".tsv", ".xlsx", ".xls", ".parquet", ".json", ".npy", ".npz"} | ||
| plot_ext = {".png", ".jpg", ".jpeg", ".svg", ".pdf", ".html"} | ||
| report_names = {"report.html"} |
There was a problem hiding this comment.
When ProtWise calls common.utils.display.create_report(out_dir) it writes a model-specific filename such as Protein-wise_report.html, not report.html, so this exact-name check routes that HTML report through the generic .html plot branch instead of reports/. In normal ProtWise runs the dashboard Reports tab therefore stays empty even though the report was generated; treat *_report.html/HTML report stems as reports or pass a standard report filename.
Useful? React with 👍 / 👎.
| tfopt.callback(mode=tf_mode, conf=tf_conf, outdir=(outdir / "tfopt" if outdir else None)) | ||
| kinopt.callback(mode=kin_mode, conf=kin_conf, outdir=(outdir / "kinopt" if outdir else None)) | ||
| model.callback(conf=model_conf, outdir=(outdir / "protwise" if outdir else None)) |
There was a problem hiding this comment.
For dashboard phoskintime-all runs, the launcher opens built.outdir after success, but these callbacks write the actual TFOpt/KinOpt/ProtWise outputs under outdir/tfopt, outdir/kinopt, and outdir/protwise. The parent run directory only has launcher provenance, and discover_result_directory() does not recurse into arbitrary child workflow directories, so the successful all-workflow run appears to have no tables/plots/reports unless the user manually finds a nested folder; aggregate/copy the child results or have the dashboard present/open those child directories.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aafe615f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ), | ||
| required_inputs=("kinase network", "TF network", "MS/protein data", "RNA data", "phospho data"), | ||
| input_assignments=( | ||
| InputSpec("config", "Config file", "conf", "Optional networkmodel config file.", extensions=(".toml", ".yaml", ".yml", ".json")), |
There was a problem hiding this comment.
When a dashboard user assigns a .yaml, .yml, or .json config here, validation accepts it and the launcher passes it as --conf, but the target runners still parse selected configs with TOML-only loaders (networkmodel.runner.load_networkmodel_config() calls load_config_toml, and ProtWise uses tomllib.load). In that supported-by-UI scenario the workflow fails before running with a TOML parse error; restrict the config InputSpec extensions to .toml unless YAML/JSON loading is added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95f1b82818
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| output_dir_arg=None, | ||
| expected_output_folder="results/prep", |
There was a problem hiding this comment.
When the dashboard launches prep, build_workflow_command() still assigns and later opens results/prep/<run>, but this descriptor passes no output-directory argument to processing.cleanup. I checked processing/cleanup.py: it writes intermediate CSVs in processing/ and then copies them to kinopt/data and tfopt/data, so the dashboard result directory contains only launcher provenance and not the generated preprocessing inputs. Either copy/organize those files into built.outdir after the run or avoid presenting that directory as the prep result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b30ecc0409
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "TF/Kin counts", | ||
| "Summarise TF and kinase psite/target counts from TFOpt and KinOpt workbooks.", | ||
| "scripts/analyze_tf_kin_counts.py", | ||
| {"tfopt_xlsx": "--tfopt-xlsx", "kinopt_xlsx": "--kinopt-xlsx", "out_dir": "--out-dir"}, |
There was a problem hiding this comment.
The dashboard builds commands such as python scripts/analyze_tf_kin_counts.py --tfopt-xlsx ... --out-dir ..., but I checked scripts/analyze_tf_kin_counts.py and scripts/curve_similarity.py: neither script parses sys.argv/argparse and each ends with main() using hard-coded defaults. When users run these generated Advanced analyses commands, their selected workbooks/output directory are ignored and the scripts read/write the default data/... and results_scripts locations instead.
Useful? React with 👍 / 👎.
| "Mechanistic insights", | ||
| "Run the existing mechanistic insights script; inputs are passed through its CLI.", | ||
| "scripts/mechanistic_insights.py", | ||
| {"results_dir": "--results-dir", "out_dir": "--out-dir"}, |
There was a problem hiding this comment.
This maps the mechanistic-insights task to --results-dir and --out-dir, but I checked scripts/mechanistic_insights.py: its argparse surface accepts --output-dir for the fitted-params directory plus data input flags like --kinase-net/--tf-net, and it does not define either of these flags. Any dashboard-generated command for this task therefore exits with unrecognized arguments instead of running the analysis.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Motivation
data/ode/.Description
common/results.pyprovidingensure_result_dir,populate_standard_subdirs,write_metadata,write_command,write_resolved_config, input hashing, console-log helpers and logger attachment.--outdir/--output-diracross local workflows and wrappers by updatingkinopt.local,tfopt.local,protwise.runner,networkmodel.runner,config/config.pyandconfig/cli.pyand preserved backward-compatible defaults.tables/,plots/,logs/,reports/,artifacts/; updated exporters/plotters (plotout.py/sheetutils.py) to acceptout_dirand to write inside the run directory.metadata.json,command.txt,console.log, andconfig_resolved.yaml(when available), and implementedpopulate_standard_subdirsto mirror legacy top-level outputs into dashboard-ready subfolders while keeping legacy filenames for compatibility.data/ode/) and updated post‑processing hooks to point at the selectedout_dir.docs/Documentation/scripts.mdanddocs/dashboard_build_plan.md) and added lightweight tests validating the result contract and CLI changes (tests/test_result_contract.py).Testing
PYTHONPATH=. pytest tests/test_result_contract.py -qand all tests passed (4 passed).python -m py_compileon modified modules to ensure syntax correctness and rangit diff --checkwith no issues.pixi run -e dev testcould not be executed in this environment because thepixiexecutable is not available; all other automated checks listed above succeeded.Codex Task