Custom Eval - Upload

[w-javed]

No description provided.

[github-actions]

API Change Check

APIView identified API level changes in this PR and created the following API reviews

azure-ai-projects

[nagkumar91]

Code Review

🔴 overwrite=True silently ignored (High)

Both samples pass overwrite=True to upload(), but the method signature is:

def upload(self, name, evaluator_version, *, folder, connection_name=None, **kwargs):

overwrite is not explicitly defined — it gets swallowed by **kwargs and forwarded to container_client.upload_blob() where it has no effect. Users will believe overwrite is supported when it isn't. If a version already exists, create_version() will likely fail with no overwrite handling.

Fix: Either implement overwrite logic (check if version exists, delete first) or remove overwrite=True from the samples.

---

🟡 Race condition in version auto-increment (Medium)

_get_next_version() fetches all existing versions, finds the max, and returns max+1. Between that call and the subsequent create_version(), another client could create the same version — classic TOCTOU race. Two concurrent upload() calls for the same evaluator name will both compute the same next version, and the second will fail with a conflict.

Fix: Document this as a known limitation, add retry logic on conflict, or have the service assign versions atomically.

---

🟡 Input mutation side effect (Medium)

upload() mutates the caller's evaluator_version object by setting definition.blob_uri:

if isinstance(evaluator_version, dict):
    definition["blob_uri"] = blob_uri  # mutates caller's dict
else:
    evaluator_version.definition.blob_uri = blob_uri  # mutates caller's object

This is undocumented and surprising. If a caller reuses the same EvaluatorVersion for multiple uploads, they'll get unexpected behavior.

Fix: Work on a deep copy of the input, or document the mutation clearly in the docstring.

---

🟡 Blocking I/O in async method (Medium)

The async version in _patch_evaluators_async.py uses synchronous os.walk() and open() inside the event loop:

async def upload(...):
    for root, dirs, files in os.walk(folder):  # blocking
        with open(file=file_path, mode="rb") as data:  # blocking
            await container_client.upload_blob(...)

This blocks the entire event loop for directory traversal and file reads.

Fix: Use asyncio.to_thread() to offload file I/O, or use aiofiles.

---

🟡 Missing async wiring in aio/operations/_patch.py (Medium)

The diff only patches the sync operations/_patch.py. The async aio/operations/_patch.py still imports BetaEvaluatorsOperations from the generated code (line 18), so the async client won't expose the new upload() method.

Fix: Apply the same patching to aio/operations/_patch.py — import from _patch_evaluators_async and override self.evaluators in __init__.

---

🟠 Unused variable output_version (Low)

In both sync and async upload():

container_client, output_version, blob_uri = self._start_pending_upload_and_get_container_client(...)

output_version is assigned but never used — dead code.

Fix: Return only 2 values from _start_pending_upload_and_get_container_client() or use _ for the unused variable.

---

🟠 No async tests (Low-Medium)

All 450 lines of tests cover the sync EvaluatorsOperations only. There is zero test coverage for the async path in _patch_evaluators_async.py.

[w-javed]

Code Review

🔴 overwrite=True silently ignored (High)

Both samples pass overwrite=True to upload(), but the method signature is:

def upload(self, name, evaluator_version, *, folder, connection_name=None, **kwargs):

overwrite is not explicitly defined — it gets swallowed by **kwargs and forwarded to container_client.upload_blob() where it has no effect. Users will believe overwrite is supported when it isn't. If a version already exists, create_version() will likely fail with no overwrite handling.

Fix: Either implement overwrite logic (check if version exists, delete first) or remove overwrite=True from the samples.

🟡 Race condition in version auto-increment (Medium)

_get_next_version() fetches all existing versions, finds the max, and returns max+1. Between that call and the subsequent create_version(), another client could create the same version — classic TOCTOU race. Two concurrent upload() calls for the same evaluator name will both compute the same next version, and the second will fail with a conflict.

Fix: Document this as a known limitation, add retry logic on conflict, or have the service assign versions atomically.

🟡 Input mutation side effect (Medium)

upload() mutates the caller's evaluator_version object by setting definition.blob_uri:

if isinstance(evaluator_version, dict):
    definition["blob_uri"] = blob_uri  # mutates caller's dict
else:
    evaluator_version.definition.blob_uri = blob_uri  # mutates caller's object

This is undocumented and surprising. If a caller reuses the same EvaluatorVersion for multiple uploads, they'll get unexpected behavior.

Fix: Work on a deep copy of the input, or document the mutation clearly in the docstring.

🟡 Blocking I/O in async method (Medium)

The async version in _patch_evaluators_async.py uses synchronous os.walk() and open() inside the event loop:

async def upload(...):
    for root, dirs, files in os.walk(folder):  # blocking
        with open(file=file_path, mode="rb") as data:  # blocking
            await container_client.upload_blob(...)

This blocks the entire event loop for directory traversal and file reads.

Fix: Use asyncio.to_thread() to offload file I/O, or use aiofiles.

🟡 Missing async wiring in aio/operations/_patch.py (Medium)

The diff only patches the sync operations/_patch.py. The async aio/operations/_patch.py still imports BetaEvaluatorsOperations from the generated code (line 18), so the async client won't expose the new upload() method.

Fix: Apply the same patching to aio/operations/_patch.py — import from _patch_evaluators_async and override self.evaluators in __init__.

🟠 Unused variable output_version (Low)

In both sync and async upload():

container_client, output_version, blob_uri = self._start_pending_upload_and_get_container_client(...)

output_version is assigned but never used — dead code.

Fix: Return only 2 values from _start_pending_upload_and_get_container_client() or use _ for the unused variable.

🟠 No async tests (Low-Medium)

All 450 lines of tests cover the sync EvaluatorsOperations only. There is zero test coverage for the async path in _patch_evaluators_async.py.

Resolved