fixed the prompt asset casting

[LinoGiger]

No description provided.

[claude]

Code Review

This PR fixes prompt asset parsing by updating imports to the new IAssetModel*/IMetadataModel* wrapper types and adding handling for originalFilename metadata alongside the existing sourceUrl path. The intent is correct, but three confirmed issues are worth addressing before merge.

---

src/rapidata/rapidata_client/benchmark/rapidata_benchmark.py

[Bug] Line 112 — Round-trip value drift after re-fetch

add_prompts caches the caller-supplied string (e.g. "/tmp/photo.jpg") directly into __prompt_assets. After accessing .english_prompts (which clears and re-fetches), __instantiate_prompts repopulates the list using instance.original_filename — just the base filename returned by the server (e.g. "photo.jpg"). The same asset now returns different values before and after any re-fetch:

# Before re-fetch
benchmark.prompt_assets  # → ["/tmp/photo.jpg"]
# After accessing .english_prompts (triggers __instantiate_prompts)
benchmark.prompt_assets  # → ["photo.jpg"]

This breaks idempotency for callers who use .prompt_assets as input to a subsequent add_prompts call or any other downstream operation.

---

[Bug] Line 112 — prompt_assets returns bare filenames mixed with URLs without any discriminator

When originalFilename metadata is present, instance.original_filename is appended — a bare filename string like "photo.jpg", not a URL. The return type is still list[str | None] with no way to distinguish a URL entry from a filename entry.

Concretely: the asset uploader in _asset_uploader.py uses a regex ^https?:// to branch between URL-upload and local-file-upload paths. A caller who reads .prompt_assets and passes the result back to add_prompts will have "photo.jpg" treated as a local filesystem path, almost certainly causing a file-not-found error or a garbage upload — silently, since the type annotation gives no hint.

Consider wrapping the return in a small named type (e.g. a dataclass with value and kind: Literal["url", "filename"]) or at minimum documenting that entries may be bare filenames and are not safe to round-trip back into add_prompts.

---

[Bug] Lines 102–105, 108–111 — Opaque AssertionError when actual_instance is None

IMetadataModel.actual_instance is declared Optional[...] and defaults to None. If the LazyValidatedModel._lazy_construct path is taken (validation failure on the outer wrapper), the object is built via model_construct() without setting actual_instance, leaving it None. The subsequent bare assert:

instance = source_url.actual_instance   # None
assert isinstance(instance, IMetadataModelSourceUrlMetadataModel)  # AssertionError, no message

...produces an opaque crash that tells the caller nothing about what the server actually returned or which field failed. Prefer an explicit check:

if not isinstance(instance, IMetadataModelSourceUrlMetadataModel):
    raise ValueError(
        f"Expected IMetadataModelSourceUrlMetadataModel for 'sourceUrl', got {type(instance)}"
    )

The same applies to the originalFilename branch (lines 108–111) and the outer file_asset check (line 98). Note also that all three assert isinstance calls are silently stripped under python -O.

---

[Minor] Line 114 — Silent None for unrecognised metadata

Previously, assets without a sourceUrl would raise an AssertionError immediately (noisy but visible). Now they silently append None. If the server starts returning a new metadata key that this code doesn't know about, the asset is quietly dropped with no log line. A logger.warning(...) here would aid future debugging.

[claude]

Code Review

This PR fixes prompt asset casting in the benchmark by switching from FileAssetModel/SourceUrlMetadataModel to the new IAssetModel*/IMetadataModel* types, and extends the metadata lookup to handle both sourceUrl (remote) and originalFilename (local-file) assets. The __normalize_cached_asset helper normalises the in-memory cache after add_prompts so it matches what a server re-fetch would produce.

---

Findings

1. Silent None for unrecognised metadata keys — no diagnostic emitted
rapidata_benchmark.py:116

When a file asset exists but contains neither sourceUrl nor originalFilename, the asset is silently appended as None with no log, warning, or error:

else:
    self.__prompt_assets.append(None)

If the server adds a new metadata key (e.g. s3Url), every prompt asset will silently become None in the cache, and downstream consumers will see missing assets with no indication of why. At minimum a logger.warning(...) should fire here.

---

2. os.path.basename does not strip Windows-style paths on Linux
rapidata_benchmark.py:141

return os.path.basename(asset)

On Linux, os.path.basename("C:\\Users\\user\\image.jpg") returns the entire string unchanged — the backslash is not a path separator. The idempotency guarantee the PR is trying to establish (cached value matches re-fetch) would silently break for any caller running on Windows and supplying an absolute local path. pathlib.PurePath(asset).name (or PureWindowsPath) handles cross-platform separators correctly.

---

3. Caller-visible behaviour change: prompt_assets returns basename after add_prompts
rapidata_benchmark.py:412–414

self.__prompt_assets.append(
    self.__normalize_cached_asset(uploaded.prompt_asset)
)

After add_prompts("/path/to/image.jpg"), a caller reading benchmark.prompt_assets[i] receives "image.jpg", not the path they supplied. This is a breaking change for any code that captures prompt_assets to reuse the original value (re-display, re-upload, path resolution). The PR description doesn't call this out, and it isn't guarded by any deprecation notice.

---

4. Third independent copy of the URL detection regex
rapidata_benchmark.py:126

__URL_SCHEME_RE = re.compile(r"^https?://", re.IGNORECASE)

The same pattern already lives at _asset_uploader.py:187 (_URL_SCHEME_RE) and _asset_upload_orchestrator.py:119 (_URL_SCHEME_RE). The comment on this copy even says "same detection the asset uploader uses." If the rule ever changes (adding ftp://, s3://, etc.), all three sites must be updated in sync. A module-level constant in a shared _url_utils.py — or exposing the one already on AssetUploader — removes the divergence risk.

---

5. @classmethod when @staticmethod is appropriate
rapidata_benchmark.py:128

@classmethod
def __normalize_cached_asset(cls, asset: str | None) -> str | None:
    if asset is None or cls.__URL_SCHEME_RE.match(asset):
        return asset
    return os.path.basename(asset)

cls is used solely to access __URL_SCHEME_RE, a compile-time constant that doesn't vary by subclass. @classmethod misleads readers into thinking this is polymorphic. Additionally, because of Python name-mangling, cls.__URL_SCHEME_RE inside this method resolves to _RapidataBenchmark__URL_SCHEME_RE regardless of what cls is — making the @classmethod polymorphism entirely illusory. A @staticmethod (with _URL_SCHEME_RE at module level or as a plain _ prefixed class attribute) is the correct shape here.

---

Overall the fix is on the right track — handling both metadata key variants is clearly necessary. The most important issues to address before merge are #1 (silent data loss) and #3 (breaking caller contract). #2 is a latent cross-platform bug. #4 and #5 are cleanups.