-
Notifications
You must be signed in to change notification settings - Fork 48
Pasha/one task per worker #4167
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
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| backend_url = os.environ.get("BACKEND_URL", DEV_STATS_SERVER_URI) | ||
| docker_image = os.environ.get("DOCKER_IMAGE", "metta-policy-evaluator-local:latest") | ||
| poll_interval = float(os.environ.get("POLL_INTERVAL", "5")) | ||
| worker_idle_timeout_minutes = float(os.environ.get("WORKER_IDLE_TIMEOUT", "60")) | ||
| task_timeout_minutes = float(os.environ.get("TASK_TIMEOUT_MINUTES", "60")) | ||
| machine_token = os.environ["MACHINE_TOKEN"] |
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.
Orchestrator ignores chart-configured timeout env var
main() now reads TASK_TIMEOUT_MINUTES (line 163), but the Helm deployment still injects WORKER_IDLE_TIMEOUT (see devops/charts/orchestrator/templates/deployment.yaml lines 22-31). In production the orchestrator will therefore always fall back to the hardcoded 60‑minute default and ignore the timeout value configured in values.yaml, defeating timeout tuning for running tasks.
Useful? React with 👍 / 👎.
| # Install dependencies in the versioned checkout | ||
| logger.info("Installing dependencies in versioned checkout...") | ||
| self._run_cmd_from_versioned_checkout( | ||
| ["uv", "run", "metta", "configure", "--profile=softmax-docker"], | ||
| capture_output=True, | ||
| ) | ||
| self._run_cmd_from_versioned_checkout( | ||
| ["uv", "run", "metta", "install"], | ||
| ) | ||
| self._run_setup_cmd(["uv", "run", "metta", "configure", "--profile=softmax-docker"]) |
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.
Worker checkout skips dependency installation
_setup_versioned_checkout now only runs uv run metta configure after fetching the target ref and no longer calls metta install. That means each worker executes tasks with whatever packages are baked into the base image rather than the dependencies declared by the fetched commit, so any task whose requirements changed since the image build will now fail or run with mismatched libraries.
Useful? React with 👍 / 👎.
|
so simple now! |
| self._run_setup_cmd(["uv", "run", "metta", "configure", "--profile=softmax-docker"]) | ||
|
|
||
| logger.info(f"Successfully set up versioned checkout at {self._versioned_path}") | ||
| logger.info(f"Successfully set up versioned checkout at {self._workdir}") |
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.
Missing dependency installation step. The old code (lines 181-183) ran both metta configure AND metta install, but the new code only runs metta configure. This will cause tasks to fail because dependencies won't be installed.
self._run_setup_cmd(["uv", "run", "metta", "configure", "--profile=softmax-docker"])
self._run_setup_cmd(["uv", "run", "metta", "install"]) # Add this line| self._run_setup_cmd(["uv", "run", "metta", "configure", "--profile=softmax-docker"]) | |
| logger.info(f"Successfully set up versioned checkout at {self._versioned_path}") | |
| logger.info(f"Successfully set up versioned checkout at {self._workdir}") | |
| self._run_setup_cmd(["uv", "run", "metta", "configure", "--profile=softmax-docker"]) | |
| self._run_setup_cmd(["uv", "run", "metta", "install"]) | |
| logger.info(f"Successfully set up versioned checkout at {self._workdir}") |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Co-authored-by: Paul Tsier <paul@elementanalytics.com>
No description provided.