[refactor]: Refactor IssueResolver and SandboxConfig for Better Modularity and Testability #8516
Conversation
…update imports - Moved the entire implementation of IssueResolver from resolve_issue.py to a new file issue_resolver.py. - Updated import statements in test_resolve_issue.py to reflect the new location of IssueResolver. - Adjusted mock patches in the test file to point to the correct module for os.getuid and get_unique_uid.
…and add build method for argument parsing
…rgs to resolve_issue.py and update tests accordingly
…an up test function signatures
…class and streamline setup_sandbox_config
…ethod tests for various scenarios
…oxContainerConfig and update related tests
|
I have prepared a sample repository as shown below: You can see that the modified code is being installed in the following line: Here is the actual issue that was created: Here is the result of the execution: And here is the pull request: |
…th build_from_args for improved clarity and consistency
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Thank you for the work on this!
Instead of a new SandboxContainerConfig class, could we use its arguments for the real SandboxConfig creation?
I'll come back on this, I'm sorry, due to the move the diff is not easy to read directly 😅
|
@enyst
I understand that this would be difficult. The SandboxContainerConfig was implemented as a value object specifically for creating the SandboxConfig used by IssueResolver. In that context, there's logic that throws an error when both runtime_container_image and base_container_image are specified at the same time: I'm not certain whether this constraint should apply globally to all uses of SandboxConfig, or if it's specific to IssueResolver. If it's a valid constraint across all usages, then it would make sense to move that validation logic into SandboxConfig itself. Alternatively, SandboxContainerConfig simply exists to satisfy that constraint and to provide a default value for runtime_container_image tailored for IssueResolver:
No worries at all — I understand the diff is hard to read right now. To better understand the refactoring, I recommend checking the commit history instead: Source code was splited here: Then I applied the validation logic change to SandboxContainerConfig here: |
There was a problem hiding this comment.
Sorry, I don't think this will work: we had an issue the other day, these tests were failing when we tried to release a new version of openhands.
I think we shouldn't hardcode the (real) version, nor the user (e.g. on my Mac the user_id here is 502) so the test is failing locally.
There was a problem hiding this comment.
Sorry, I don't think this will work: we had an issue the other day, these tests were failing when we tried to release a new version of openhands.
I'm sorry for causing issues that affected the release.
I think we shouldn't hardcode the (real) version, nor the user (e.g. on my Mac the user_id here is 502) so the test is failing locally.
I've modified the version information so that it works with a minimal mock instead of being hardcoded.
As for the user_id, I updated the assert_sandbox_config function so that it only performs the check when user_id is explicitly specified.
By the way, regarding the user_id — do you think there's an issue in the implementation of SandboxConfig itself?
https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/core/config/sandbox_config.py#L56
My original code was written to retrieve the default value:
kotauchisunsun@31b2f3c#diff-fd80cef1f8ee36908d45fd51ef9879d90c0cfb53c669c8362ac4a9160fec2a8dR15
So I assumed that the test wouldn't fail depending on the OS.
The os.getuid function appears to be available on macOS as well:
https://docs.python.org/3.12/library/os.html#os.getuid
Do you have any idea why this issue occurred? I'd really appreciate it if you could share the reason.
There was a problem hiding this comment.
The test was testing against 1000, I think. getuid works, and it returns my user id on my local system when run locally. I think we simply can't assume that it will fallback to 1000, because it won't necessarily?
There was a problem hiding this comment.
As a result of attempting several experiments to solve this issue, I found that it is difficult to resolve.
I had assumed that in assert_sandbox_config, I could retrieve SandboxConfig.model_fields['user_id'].default and use it to validate the user_id.
However, the SandboxConfig class inherits from Pydantic's BaseModel and assigns a value to user_id as part of the "class definition".
https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/core/config/sandbox_config.py#L56
Due to the mechanism of inheritance in Pydantic, the value for user_id is likely assigned at the time the module is loaded.
As a result, I discovered that mocking the value does not work properly for os.getuid.
Although it may be technically possible to override the value with a more complex procedure, I decided not to pursue that path, as it could make the test code unstable.
|
Just to note, I don't think we need to enforce that those two images are mutually exclusive. They are, but other entry points to the application, as you pointed out, don't enforce it with an error; instead, I think they prefer the Maybe we can do the same? |
|
@enyst
I'm going to exclude the controversial parts and proceed with these steps, what do you think? |
|
Up to you. Thank you for this work! |
2 participants
End-user friendly description of the problem this fixes or functionality this introduces.
Summarize what the PR does, explaining any non-trivial design decisions.
Link of any specific issues this addresses: