BWDO-836 restrict docker run cpu usage - #69
Open
BenjiMilan wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a host-side CPU affinity limit to sc docker runs (on high-core machines) and updates Rich-markup log strings in branching commands to avoid Python 3.12+ invalid escape warnings.
Changes:
- Add
--cpuset-cpus=0-Nto generateddocker runcommands when the host has many CPUs. - Introduce a helper to compute a max CPU ID that leaves 4 CPUs free.
- Convert markup strings to raw f-strings / escaped literals to avoid malformed escape warnings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/sc/docker/docker.py |
Adds CPU pinning to generated docker run args and a helper to compute the limit. |
src/sc/branching/commands/show.py |
Updates Rich markup strings to avoid invalid escape warnings. |
src/sc/branching/commands/group.py |
Updates Rich markup string to avoid invalid escape warnings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+586
to
+587
| if max_cpu := self._get_cpu_limit(): | ||
| docker_args += [f"--cpuset-cpus=0-{max_cpu}"] |
Comment on lines
+687
to
+693
| cpu_count = os.cpu_count() or 1 | ||
|
|
||
| if cpu_count >= 20: | ||
| # We leave 4 cores free, but CPU IDs start from zero so minus 5 | ||
| return cpu_count - 5 | ||
| else: | ||
| return None |
| group_str = " ".join([rf"\[[bold yellow]{g}[/]]" for g in groups]) | ||
| else: | ||
| group_str = "\[[red bold]No Groups[/]]" | ||
| group_str = "\\[[red bold]No Groups[/]]" |
Contributor
Author
There was a problem hiding this comment.
To be completely honest when I did it with a raw string only and not an rf" string my vscode syntax turns the whole string red which looked horrible so I did it this way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Restrict docker run cpu usage.
The random other changes are I was testing this change in a py3.13 venv and apparently since py3.12 it warns you about malformed escapes where \[ is invalid as you can't just escape a [. So either \\[ is good as then logger sees it as \[ or using a raw string rf"" also works. (The code still worked anyway but just suppressing warnings)
Also in a case like this would you ever bother creating a github issue that links to jira or something? I recall we should be using issues instead in rdkcentral but not sure how important it is if we already have a jira ticket made