Feature/create report - #2
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive reporting configuration system by adding various YAML configuration files for Hydra, MLFlow tracking, mask retrievers, and report storage. It also integrates the report library as a new dependency in pyproject.toml and uv.lock, along with its required sub-dependencies. I have no feedback to provide as there were no review comments to assess.
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive reporting configuration system by adding multiple YAML configuration files for MLflow tracking URI setup, reporter masking and save strategies, and validation heatmap examples. It also adds a new Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pyproject.toml (1)
25-25: Clarify the purpose of thereportdependency group.The
reportpackage is already listed as a direct dependency in[project].dependencies(line 25), making it installed by default. The separate[dependency-groups].reportgroup (line 33) containing only["report"]appears redundant unless there's a specific use case (e.g., making it optional in the future).If the intent is to keep
reportas an optional dependency, consider removing it from[project].dependenciesand only keeping it in the dependency group.Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 25, The pyproject declares "report" both in [project].dependencies and in [dependency-groups].report, which is redundant; decide whether "report" should be a default dependency or optional—if optional, remove "report" from [project].dependencies and keep it only in [dependency-groups].report, otherwise delete the [dependency-groups].report entry so "report" remains a normal required dependency; update the manifest accordingly to reflect that decision.configs/reporting/reporter/validation_heatmap/virchow2_level1.yaml (1)
10-11: Avoid hardcoded local filesystem paths in shared config.Line 10 makes this run config host-specific and brittle across environments. Prefer a required placeholder (or env interpolation) and pass it via override.
Proposed refactor
background: _target_: report.masks.BasicImageRetriever - source_dir: /mnt/data/MOU/lymph_nodes/dataset1-ihc-2023/ + source_dir: ??? globs: ["SNB_IHC_CASE_1_SLIDE_3-1.mrxs"] layer_name: background🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/reporting/reporter/validation_heatmap/virchow2_level1.yaml` around lines 10 - 11, Replace the hardcoded filesystem path in the config key source_dir with a required placeholder or environment interpolation (e.g. ${SOURCE_DIR} or ${env:SOURCE_DIR}) and keep globs as-is; update the config consumer/loader to treat source_dir as mandatory and fail fast with a clear message if the placeholder/env var is unset so callers pass an override or set the env before running.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/reporting/default.yaml`:
- Around line 1-4: The reporting/default.yaml file is not included in the active
Hydra defaults chain; add "reporting/default" to the defaults list so the
reporting config is reachable at runtime — update the defaults array in
configs/base.yaml (or alternatively add it to the defaults in
configs/lymph_nodes.yaml or configs/preprocessing.yaml) so that the defaults
chain contains the entry "reporting: default" (i.e., include reporting/default)
and thus wires the reporting stack into the application's Hydra configuration
tree.
---
Nitpick comments:
In `@configs/reporting/reporter/validation_heatmap/virchow2_level1.yaml`:
- Around line 10-11: Replace the hardcoded filesystem path in the config key
source_dir with a required placeholder or environment interpolation (e.g.
${SOURCE_DIR} or ${env:SOURCE_DIR}) and keep globs as-is; update the config
consumer/loader to treat source_dir as mandatory and fail fast with a clear
message if the placeholder/env var is unset so callers pass an override or set
the env before running.
In `@pyproject.toml`:
- Line 25: The pyproject declares "report" both in [project].dependencies and in
[dependency-groups].report, which is redundant; decide whether "report" should
be a default dependency or optional—if optional, remove "report" from
[project].dependencies and keep it only in [dependency-groups].report, otherwise
delete the [dependency-groups].report entry so "report" remains a normal
required dependency; update the manifest accordingly to reflect that decision.
🪄 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: 9870dd68-22ca-44cc-a069-8ed695e0363e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
configs/reporting/default.yamlconfigs/reporting/hydra/default.yamlconfigs/reporting/mlflow/base.yamlconfigs/reporting/mlflow/kubas_cluster.yamlconfigs/reporting/mlflow/kubas_cluster_s3.yamlconfigs/reporting/mlflow/kubas_external.yamlconfigs/reporting/mlflow/local.yamlconfigs/reporting/reporter/background/drive.yamlconfigs/reporting/reporter/background/mlflow_artifact.yamlconfigs/reporting/reporter/background/mlflow_run_id.yamlconfigs/reporting/reporter/background/mlflow_tyler.yamlconfigs/reporting/reporter/save/local.yamlconfigs/reporting/reporter/save/mlflow_attach_to_run.yamlconfigs/reporting/reporter/save/mlflow_standalone_run.yamlconfigs/reporting/reporter/validation_heatmap/virchow2_level1.yamlpyproject.toml
| defaults: | ||
| - _self_ | ||
| - hydra: default | ||
| - mlflow: kubas_cluster |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Hydra entrypoints =="
rg -nP --type=py '@hydra\.main\s*\(|config_name\s*=|config_path\s*=' -C2
echo
echo "== References to reporting/default and reporting groups =="
rg -nP 'reporting/default|configs/reporting|reporter/validation_heatmap|mlflow_standalone_run|mlflow_artifact' -C2
echo
echo "== Root defaults files =="
fd -i 'base.yaml|lymph_nodes.yaml|default.yaml' configs --exec sh -c 'echo "--- $1"; sed -n "1,120p" "$1"' sh {}Repository: RationAI/lymph-nodes
Length of output: 3230
Wire reporting/default.yaml into the active config tree.
configs/reporting/default.yaml is not reachable from any of the application's Hydra entrypoints. The root configs (lymph_nodes, preprocessing) do not include reporting/default in their defaults chains. Without this wiring, the reporting stack cannot be used at runtime, even though the configuration files are present.
Add reporting/default to the defaults chain in configs/base.yaml or to the relevant entrypoint configs (configs/lymph_nodes.yaml or configs/preprocessing.yaml).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configs/reporting/default.yaml` around lines 1 - 4, The
reporting/default.yaml file is not included in the active Hydra defaults chain;
add "reporting/default" to the defaults list so the reporting config is
reachable at runtime — update the defaults array in configs/base.yaml (or
alternatively add it to the defaults in configs/lymph_nodes.yaml or
configs/preprocessing.yaml) so that the defaults chain contains the entry
"reporting: default" (i.e., include reporting/default) and thus wires the
reporting stack into the application's Hydra configuration tree.
Summary by CodeRabbit
New Features
Chores