feat: python requirements#1128
Conversation
Requirement PR ChecklistUse this checklist when adding or modifying requirements in Base Class
Validation Logic
Integration
|
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
planetf1
left a comment
There was a problem hiding this comment.
Two of the four exported requirements don't behave as documented. OutputSizeLimit always passes regardless of output volume, and ImportRestrictions([]) allows all imports instead of blocking them all. Both bugs are masked by tautological test assertions. Requesting changes on these before merge.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
planetf1
left a comment
There was a problem hiding this comment.
Thanks for the fixes — I've gone through the latest version against the original push. A few observations from the refactor:
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
| ) | ||
|
|
||
| code = extraction_result.reason | ||
| assert code is not None |
There was a problem hiding this comment.
Nit: assert gets stripped when Python runs with -O, so if _has_python_code_listing ever returned a None reason on success this would silently blow up rather than give a useful message. Same pattern at lines 144 and 223. In practice the contract is stable and -O is rarely used, so this is low risk — but an explicit if code is None: return ValidationResult(result=False, reason=...) would be more robust.
There was a problem hiding this comment.
fixed them and another one in python_req.py
| def __init__( | ||
| self, | ||
| limit_chars: int = 10_000, | ||
| timeout: int = 5, |
There was a problem hiding this comment.
Nit: limit_chars is validated to be positive but timeout isn't. A zero or negative value would cause subprocess.TimeoutExpired immediately, which the except below catches and turns into a failure result — so nothing actually breaks. Just an inconsistency worth tidying up.
There was a problem hiding this comment.
value check is added
| result=False, | ||
| reason=f"Output size ({output_size} chars) exceeds limit ({self.limit_chars}).", | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
Nit: catching Exception broadly means any unexpected error (misconfigured environment, attribute error, etc.) silently becomes "Error checking output size". The code fails closed so there's no safety concern, but it makes debugging harder. A logger.exception(...) call here before the return would help surface those. As a bonus logger on line 26 is currently unused — this would be its first call.
There was a problem hiding this comment.
loggger statement is added
| reqs = python_tool_requirements(timeout_seconds=15) | ||
| execution_req = reqs[2] | ||
| assert isinstance(execution_req, PythonExecutionReq) | ||
| assert execution_req._timeout == 15 |
There was a problem hiding this comment.
Nit: this and the two tests below check ._timeout, ._use_sandbox, and ._allowed_imports directly on PythonExecutionReq. These work fine today but they're implementation details — if that class changes how it stores those values internally the tests break without any API change. Testing observable behaviour (e.g. a short timeout actually causing a timeout result) would be more resilient, though that's a higher bar for a propagation check.
There was a problem hiding this comment.
These 3 tests are replaced by tests that don't use the internal variables.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Pull Request
Issue
Fixes #1119, #1121
Description
Add Python code generation requirements
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.