-
Notifications
You must be signed in to change notification settings - Fork 47
fix: pre-build SDK sdist once to eliminate 500-image build regression (#504) #505
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,9 @@ | |
| --image ghcr.io/openhands/eval-agent-server --target source-minimal | ||
| """ | ||
|
|
||
| # Use stdlib logging instead of openhands.sdk.get_logger to avoid initializing | ||
| # Rich console state before ProcessPoolExecutor forks (causes deadlocks). | ||
| import logging | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
|
|
@@ -23,10 +26,9 @@ | |
| ) | ||
| from benchmarks.utils.dataset import get_dataset | ||
| from benchmarks.utils.image_utils import remote_image_exists | ||
| from openhands.sdk import get_logger | ||
|
|
||
|
|
||
| logger = get_logger(__name__) | ||
| logger = logging.getLogger(__name__) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Important: You replaced openhands logger with stdlib logging, but where is Check if there's a central initialization point, or add basic config in the main entry points. |
||
| WRAPPER_DOCKERFILE = Path(__file__).with_name("Dockerfile.swebench-deps") | ||
|
|
||
|
|
||
|
|
||
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.
🟡 Suggestion: This disk check has nested conditionals and multiple fallback strategies. It works, but consider extracting to a script file for clarity.
That said, for one-off infrastructure tooling, this level of inline complexity is acceptable - not worth blocking the PR over.