Skip to content

Add support for template, configuration, and evaluation file paths#266

Open
spateluf04 wants to merge 1 commit into
Udayraj123:masterfrom
spateluf04:feature-cli-config-args
Open

Add support for template, configuration, and evaluation file paths#266
spateluf04 wants to merge 1 commit into
Udayraj123:masterfrom
spateluf04:feature-cli-config-args

Conversation

@spateluf04
Copy link
Copy Markdown

Feature: Support CLI arguments for config files

Adds support for passing config files

*templateFile
*configFile
*evaluationFile

Users can now specifically choose JSON paths instead of only on detection from the directory

Closes #201

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add CLI arguments for custom configuration file paths

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add CLI arguments for custom template, config, and evaluation file paths
• Allow users to override default file detection with explicit paths
• Support -tF/--templateFile, -cF/--configFile, -eF/--evaluationFile flags
• Update file path resolution logic to prioritize CLI arguments over directory detection
Diagram
flowchart LR
  CLI["CLI Arguments<br/>-tF, -cF, -eF"]
  Logic["Path Resolution<br/>Logic"]
  Default["Default Directory<br/>Detection"]
  Files["Template, Config,<br/>Evaluation Files"]
  
  CLI -- "if provided" --> Logic
  Default -- "if not provided" --> Logic
  Logic -- "resolves to" --> Files
Loading

Grey Divider

File Changes

1. main.py ✨ Enhancement +27/-0

Add CLI arguments for file path configuration

• Add three new CLI arguments: --templateFile, --configFile, --evaluationFile
• Each argument has short flag (-tF, -cF, -eF) and destination variable
• Arguments are optional string type parameters for specifying custom file paths

main.py


2. src/entry.py ✨ Enhancement +15/-3

Implement CLI argument path override logic

• Update process_dir() function to check CLI arguments before using default paths
• Modify config path resolution to use args["config_path"] if provided
• Modify template path resolution to use args["template_path"] if provided
• Modify evaluation path resolution to use args["evaluation_path"] if provided

src/entry.py


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Missing invalid-path error handling 📘 Rule violation ⛯ Reliability
Description
When a user supplies a CLI file path that does not exist, the code silently skips loading
config/template/evaluation instead of emitting an actionable error. This violates robust error
handling and makes failures hard to diagnose.
Code

src/entry.py[R88-104]

+    local_config_path = (
+        Path(args["config_path"])
+        if args.get("config_path")
+        else curr_dir.joinpath(CONFIG_FILENAME)
+    )
    if os.path.exists(local_config_path):
        tuning_config = open_config_with_defaults(local_config_path)

    # Update local template (in current recursion stack)
-    local_template_path = curr_dir.joinpath(TEMPLATE_FILENAME)
+    local_template_path = (
+        Path(args["template_path"])
+        if args.get("template_path")
+        else curr_dir.joinpath(TEMPLATE_FILENAME)
+    )
    local_template_exists = os.path.exists(local_template_path)
    if local_template_exists:
        template = Template(
Evidence
PR Compliance ID 3 requires handling edge cases and providing actionable context; the new logic
checks os.path.exists(...) but provides no warning/error when a user-supplied path is missing,
leading to silent misconfiguration.

Rule 3: Generic: Robust Error Handling and Edge Case Management
src/entry.py[88-95]
src/entry.py[97-107]
src/entry.py[124-129]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
User-provided `--configFile`, `--templateFile`, and `--evaluationFile` paths that do not exist are silently ignored, causing confusing behavior.

## Issue Context
The program should fail fast (or at least warn) when explicitly provided paths are invalid, and include the argument name and the path in the error message.

## Fix Focus Areas
- src/entry.py[88-95]
- src/entry.py[97-107]
- src/entry.py[124-129]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Eval paths resolved wrong 🐞 Bug ✓ Correctness
Description
When --evaluationFile is provided, EvaluationConfig still resolves
answer_key_csv_path/answer_key_image_path relative to each processed directory (curr_dir) rather
than relative to the evaluation.json location. This commonly breaks multi-folder runs because
evaluation.json typically uses relative paths like "answer_key.csv" next to itself, leading to
missing-file errors or unexpected behavior.
Code

src/entry.py[R124-131]

+    local_evaluation_path = (
+        Path(args["evaluation_path"])
+        if args.get("evaluation_path")
+        else curr_dir.joinpath(EVALUATION_FILENAME)
+    )
    if not args["setLayout"] and os.path.exists(local_evaluation_path):
        if not local_template_exists:
            logger.warning(
Evidence
src/entry.py now allows supplying evaluation_path from CLI and passes (curr_dir,
local_evaluation_path) into EvaluationConfig. EvaluationConfig then joins answer key resource paths
against curr_dir. Sample evaluation.json files in the repo use relative paths (e.g.,
"answer_key.csv"), which will only resolve correctly if the base is the evaluation.json
directory—not the varying curr_dir during recursion.

src/entry.py[79-139]
src/evaluation.py[192-227]
samples/answer-key/using-csv/evaluation.json[1-6]
samples/community/UPSC-mock/evaluation.json[1-10]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When `--evaluationFile` is used, `EvaluationConfig` resolves `answer_key_csv_path` and `answer_key_image_path` relative to `curr_dir`, which changes during recursive directory processing. This breaks typical configurations where the CSV/JPG live alongside `evaluation.json` and are referenced with relative paths.

## Issue Context
Repo samples show `answer_key_csv_path: &quot;answer_key.csv&quot;` and `answer_key_image_path: &quot;answer_key.jpg&quot;` as relative paths.

## Fix Focus Areas
- src/entry.py[124-139]
- src/evaluation.py[192-227]

## Implementation notes
- In `EvaluationConfig.__init__`, compute `base_dir = Path(evaluation_path).parent`.
- For `answer_key_csv_path` / `answer_key_image_path`, if the configured path is absolute use it as-is; otherwise join against `base_dir`.
- Keep `curr_dir` usage for other semantics (if any), but do not use it as the base directory for evaluation resource files.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Evaluation created without template 🐞 Bug ⛯ Reliability
Description
process_dir may instantiate EvaluationConfig even when template is None (it only warns when a local
template file is missing). EvaluationConfig unconditionally dereferences template.global_empty_val,
causing an AttributeError crash (and a confusing stack trace) instead of a clear user-facing error.
Code

src/entry.py[R129-131]

    if not args["setLayout"] and os.path.exists(local_evaluation_path):
        if not local_template_exists:
            logger.warning(
Evidence
src/entry.py calls EvaluationConfig regardless of whether template is actually loaded. In
EvaluationConfig.__init__, template is dereferenced unconditionally while building marking schemes
(template.global_empty_val), so a missing template will crash immediately.

src/entry.py[124-139]
src/evaluation.py[298-302]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`process_dir` can create an `EvaluationConfig` even when `template` is `None`, which then crashes due to unconditional `template.global_empty_val` access.

## Issue Context
This can happen more easily now when `--evaluationFile` is provided via CLI but no template is found/loaded (or `--templateFile` points to a missing file).

## Fix Focus Areas
- src/entry.py[129-139]
- src/evaluation.py[298-302]

## Implementation notes
- In `process_dir`, before instantiating `EvaluationConfig`, add a guard:
 - if `template is None`: raise an exception like `&quot;Evaluation config provided/found but no template is loaded. Provide --templateFile or ensure template.json exists in the directory tree.&quot;`
- Optionally, for CLI-provided `--templateFile`, validate existence early and fail fast with a clear message.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment thread src/entry.py
Comment thread src/entry.py
Comment thread src/entry.py
Comment thread src/entry.py
Comment on lines +88 to +92
local_config_path = (
Path(args["config_path"])
if args.get("config_path")
else curr_dir.joinpath(CONFIG_FILENAME)
)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about passing a default arg to .get()

Suggested change
local_config_path = (
Path(args["config_path"])
if args.get("config_path")
else curr_dir.joinpath(CONFIG_FILENAME)
)
local_config_path = args.get("config_path", curr_dir.joinpath(CONFIG_FILENAME))

Comment thread main.py
)

argparser.add_argument(
"-tF",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shorthands should be single letter, multiple letter short hands are for combining multiple different shorthands

Copy link
Copy Markdown
Owner

@Udayraj123 Udayraj123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the crisp PR, can you add a testing video or even better if possible - a unit test that checks this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Support CLI arguments for passing the configuration files

2 participants