-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(api): add backward compatibility for Eval API method signatures #4683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(api): add backward compatibility for Eval API method signatures #4683
Conversation
src/llama_stack_api/eval/compat.py
Outdated
| warnings.warn( | ||
| _DEPRECATION_MESSAGE.format(method_name=method_name, request_class=request_class), | ||
| DeprecationWarning, | ||
| stacklevel=2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stacklevel=2 in _emit_deprecation_warning points to the resolver function rather than user code. The call chain is:
- _emit_deprecation_warning (stacklevel=1)
- resolve_* function (stacklevel=2) ← current target
- Router method (stacklevel=3)
- User code (stacklevel=4) ← desired target
Fix: Change stacklevel=2 to stacklevel=4 or make it configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yes. Changed stacklevel=2 to stacklevel=4 so warnings point to user code.
|
|
||
| # Old-style parameters | ||
| if benchmark_id is not None and benchmark_config is not None: | ||
| _emit_deprecation_warning("run_eval", "RunEvalRequest") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a request object is passed, it's returned immediately without validation. If someone passes RunEvalRequest(benchmark_id="", benchmark_config=None) (e.g., via Pydantic's construct()), the error surfaces deep in the router with confusing messages.
Add explicit validation when request object is provided, or rely on Pydantic validation being enforced at construction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added _validate_not_empty() checks for required fields when request objects are passed, catching empty/None values before they reach the router.
src/llama_stack_api/eval/compat.py
Outdated
| benchmark_config=benchmark_config, | ||
| ) | ||
|
|
||
| raise ValueError("Either 'request' (RunEvalRequest) or both 'benchmark_id' and 'benchmark_config' must be provided") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the valueerrors in compat:
Error messages don't indicate which parameters were actually provided vs. which are missing. Users must guess which parameter they forgot.
You can try:
raise ValueError(
f"Either 'request' or all required params must be provided. "
f"Missing: {', '.join(missing)}"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added _format_missing_params() helper that shows both missing and provided parameters in error messages.
| resolve_job_status_request(job_id="job-456") # missing benchmark_id | ||
|
|
||
|
|
||
| class TestResolveJobCancelRequest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestResolveJobCancelRequest and TestResolveJobResultRequest are missing test_missing_parameters_raises_error tests that exist for other resolvers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these tests to both TestResolveJobCancelRequest and TestResolveJobResultRequest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: No test documents what happens when BOTH a request object AND individual parameters are provided. The code silently ignores individual parameters, which could confuse users. (could be also resolved with a documentation or ignored-> nit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test_request_object_takes_precedence_over_individual_params tests and documented the behavior in docstrings.
| Args: | ||
| request: The new-style request object (preferred) | ||
| benchmark_id: (Deprecated) The benchmark ID | ||
| job_id: (Deprecated) The job ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other methods document their return type, but job_cancel does not document that it returns None (which is part of the signature).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Returns: None to the job_cancel docstring.
| RunEvalRequest, | ||
| ) | ||
|
|
||
| _DEPRECATION_MESSAGE = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standard practice is to include a target version for removal (@leseb maybe you can suggest a version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I think it'd be 0.6.0 but I'll wait for @leseb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @leseb , is it reasonable to specify 0.6.0 as target version for removal? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Empty strings pass the is not None check but create invalid requests. Consider adding validation or at least a test documenting this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old-style params now use truthiness checks (if benchmark_id and ...) which reject empty strings/lists. Request objects are explicitly validated with _validate_not_empty(). Added TestEmptyValueValidation tests.
|
Thanks for the review, @r-bit-rry ! I addressed the comments and happy to discuss any remaining concerns! |
What does this PR do?
Adds a backward compatibility layer for the Eval API to support both old-style (individual parameters) and new-style (request objects introduced in #4425) calling conventions. When using the deprecated old-style, a
DeprecationWarningis emitted.Changes:
compat.pymodule with resolver functionsEvalRouterto accept both calling stylesllama_stack_apiExample:
New style (preferred, introduced in #4425)
await eval.run_eval(RunEvalRequest(benchmark_id="...", benchmark_config=...))Old style (deprecated, emits warning - added in this PR)
await eval.run_eval(benchmark_id="...", benchmark_config=...)Test Plan
uv run pytest tests/backward_compat/test_eval_compat.py -vuv run pytest tests/unit/test_eval_models.py tests/unit/providers/nvidia/test_eval.pyuv run pre-commit run --all-filescc @leseb @r-bit-rry