Make accessing and gathering search results easier

[shaahji]

Make accessing and gathering search results easier

• Add search point information to evaluation result
• Include search point information in final printed output
• Add helper scripts to scan log and generate a csv
• Add another helper script to collect search results from evaluation results on remote storage

Checklist before requesting a review

• Add unit tests for this change.
• Make sure all tests can pass.
• Update documents if necessary.
• Lint and apply fixes to your code by running lintrunner -a
• Is this a user-facing change? If yes, give a description of this change to be included in the release notes.

(Optional) Issue link

[Copilot]

🟡 Not ready to approve

There are correctness/UX gaps (misleading script usage text, optional Azure dependency handling, and missing test coverage for newly persisted evaluation fields) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR extends Olive’s search/evaluation bookkeeping so that each evaluated model can be traced back to its originating search point, and adds scripts to extract/search results into CSVs for easier analysis.

Changes:

• Add SearchPoint.to_dict() and thread a JSON-serializable search_point payload through the engine/pass execution path.
• Persist search_point and parent_model_id into cached evaluation JSON and include search_point in run-history output.
• Add two helper scripts to extract search results from (1) local run logs and (2) Azure Blob-stored evaluation results.

File summaries

File	Description
scripts/parse_search_results.py	New script to parse run-history tables from logs and emit flattened CSV (optionally enrich with model sizes from blob).
scripts/gather_search_results.py	New script to scan evaluation JSONs in Azure Blob storage and emit flattened CSV (including model size enrichment).
olive/search/search_point.py	Add SearchPoint.to_dict() to produce a clean nested parameter/value mapping for serialization.
olive/engine/footprint.py	Extend run-history/footprint node data to carry search_point and print it in summaries.
olive/engine/engine.py	Thread search_point through pass execution; cache evaluation JSON now includes search_point and parent_model_id.

Review details

Comments suppressed due to low confidence (2)

scripts/gather_search_results.py:164

• After making Azure imports optional, scan_evaluations should raise a clear ImportError when the Azure SDK isn't available, instead of failing later with a NoneType error.

    if not subscription_id:
        raise ValueError("subscription_id is required when resolving evaluation results from blob storage")

    credential = DefaultAzureCredential()
    blob_service = BlobServiceClient(account_url=_BLOB_ACCOUNT_URL, credential=credential)

scripts/parse_search_results.py:247

• If Azure SDK dependencies are optional (per the import pattern above), _fetch_model_sizes_from_blob should fail with a clear ImportError when the user requests blob-based size enrichment without those packages installed.

    if not subscription_id:
        raise ValueError("subscription_id is required when resolving model sizes from blob storage")

    credential = DefaultAzureCredential()
    blob_service = BlobServiceClient(account_url=_BLOB_ACCOUNT_URL, credential=credential)

• Files reviewed: 5/5 changed files
• Comments generated: 5
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new Azure gather script still hard-depends on Azure SDK imports at module import time (breaking non-Azure use) and there are a couple of correctness/type-robustness issues that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (4)

scripts/gather_search_results.py:46

• The script imports Azure SDK modules at import time, which makes the script unusable for users who only want to inspect local files / read the help text unless they have azure-identity and azure-storage-blob installed. Make these imports optional and fail with a targeted error only when Azure functionality is invoked.

from azure.identity import DefaultAzureCredential
from azure.storage.blob import BlobServiceClient

scripts/gather_search_results.py:165

• After making Azure imports optional, this function should explicitly raise a clear ImportError when the Azure SDK packages are missing; otherwise it will fail later with a confusing 'NoneType is not callable' error when constructing clients.

    if not subscription_id:
        raise ValueError("subscription_id is required when resolving evaluation results from blob storage")

    credential = DefaultAzureCredential()
    blob_service = BlobServiceClient(account_url=_BLOB_ACCOUNT_URL, credential=credential)
    container_client = blob_service.get_container_client(_BLOB_CONTAINER)

olive/engine/footprint.py:30

• RunHistory field types don't match the values assigned in summarize_run_history: parent_model_id, from_pass, duration_sec, and metrics are all written as None for some nodes (e.g., the input model or models without evaluation). The annotations should be optional to reflect actual values and avoid misleading API consumers/static checks.

    parent_model_id: str
    from_pass: str
    search_point: str | None
    duration_sec: float
    metrics: str

scripts/parse_search_results.py:89

• _parse_json_cell is annotated/used as if it always returns a JSON object (dict), but json.loads can return non-dict values (e.g., null, lists). If that happens, _flatten will generate an empty-string column name and produce malformed CSV output. Treat non-dict JSON values as invalid/empty cells.

    try:
        return json.loads(text)
    except json.JSONDecodeError:
        return None

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

Footprint.record is currently gated such that common calls like record(model_id=...) become no-ops, which breaks run history and prevents the new search_point/parent_model_id propagation from working correctly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (2)

scripts/gather_search_results.py:46

• The script imports Azure SDK modules at top-level, so running any part of it fails immediately with ImportError unless azure-identity and azure-storage-blob are installed. Prefer importing these lazily (inside scan_evaluations) and raising a targeted, actionable error message so the script can at least show --help / argument errors without requiring optional deps.

from azure.identity import DefaultAzureCredential
from azure.storage.blob import BlobServiceClient

olive/engine/footprint.py:309

• This if condition is long enough to likely exceed the repo’s formatter/linter line-length limits, and it’s hard to read/maintain as a single line. Wrap it in parentheses and split it across lines so Black/lintrunner won’t churn it (and to keep future edits safe).

                    if not _v.metrics.cmp_direction or metric_name not in _v.metrics.cmp_direction or not v.metrics.cmp_direction or metric_name not in v.metrics.cmp_direction:

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

scripts/gather_search_results.py still imports Azure dependencies at module import time, which breaks basic usability (including -h) in environments without optional Azure packages and needs the lazy-import fix.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (2)

scripts/gather_search_results.py:46

• azure-identity / azure-storage-blob are imported at module import time, which prevents even python scripts/gather_search_results.py -h from working in environments that don’t have the optional Azure packages installed. Since the Azure dependency is only needed when actually scanning blob storage, import it lazily inside scan_evaluations (similar to parse_search_results.py).

from azure.identity import DefaultAzureCredential
from azure.storage.blob import BlobServiceClient

scripts/gather_search_results.py:165

• After moving Azure imports out of the module top-level, DefaultAzureCredential / BlobServiceClient should be imported (with a targeted error message) inside scan_evaluations so missing optional dependencies fail with a clear instruction.

    credential = DefaultAzureCredential()
    blob_service = BlobServiceClient(account_url=_BLOB_ACCOUNT_URL, credential=credential)
    container_client = blob_service.get_container_client(_BLOB_CONTAINER)

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Changes recommended

Critical parser incompatibility and additional metadata, serialization, and scalability issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

olive/engine/engine.py:448

• Because the dictionary expansion comes after the explicit index entry, a caller-named pass called index overwrites the global search-point index with that pass's value. Pass names are caller-provided by Engine.register and are not restricted, so the cached/run-history search point can lose its integer index. Put the explicit index after the expansion or reserve/validate this key.

                        search_point={"index": sample.search_point.index, **sample.search_point.to_dict()},

olive/engine/engine.py:448

• In pass-by-pass mode, SearchStrategy yields a SearchPoint for only the current search space; earlier choices are carried separately in sample.model_ids. Passing this object unchanged means the final node/evaluation JSON contains only the last pass's point, so the new CSV cannot reconstruct the complete search configuration. Please carry the accumulated search path into the persisted result and preserve per-sample associations when model IDs are reused.

                        search_point={"index": sample.search_point.index, **sample.search_point.to_dict()},

olive/engine/footprint.py:155

• The summarize_run_history docstring still lists only model_id, parent_model_id, from_pass, duration, and metrics, but this change adds search_point to every RunHistory row. Update the public method documentation so consumers know the printed/API output now includes this field.

                search_point=config_json_dumps(node.search_point, indent=2) if node.search_point else None,

olive/search/search_point.py:41

• This serializer is now the source of every persisted search point, but the added engine tests only pass hard-coded dictionaries and never exercise a real SearchPoint. Add a focused test with nested OrderedDict values to verify the JSON-safe shape and prevent regressions that would corrupt all run-history and evaluation metadata.

    def to_dict(self) -> dict[str, Any]:

scripts/gather_search_results.py:109

• The new Azure scanner and model-size enrichment have no automated coverage, including evaluation-blob filtering, signal flattening, malformed JSON handling, and size aggregation. Add mock-container tests for these paths because the implementation intentionally catches errors, which could otherwise produce incomplete CSVs without failing the command.

def _iter_evaluation_blobs(container_client, operation_id: str):
    """Yield (blob_name, parsed_json) for every evaluation JSON under the operation id."""
    prefix = f"{operation_id}/cache/default_workflow/evaluations/"
    for blob in container_client.list_blobs(name_starts_with=prefix):
        if not blob.name.endswith(".json"):
            continue
        try:
            data = container_client.download_blob(blob.name).readall()
            yield blob.name, json.loads(data)

scripts/parse_search_results.py:105

• This new format-sensitive parser reconstructs multiline tabulate cells and filters intermediate rows, but no tests exercise it; the added tests only cover engine cache writes. Add fixture-based tests from actual grid output, including multiline JSON and timestamped lines, and assert the resulting CSV headers and rows.

def _collect_table_rows(lines: list[str], header_idx: int) -> tuple[list[str], list[list[str]]]:
    """Parse the grid table starting at the header line.

    Returns the header column names and a list of rows, where each row is a list of
    reconstructed cell strings (multi-line cells joined with newlines).

• Files reviewed: 6/6 changed files
• Comments generated: 5
• Review effort level: Lite