feat(bug-finding): warn users if resource config conflicts with machi…#159
Open
tkqdldk wants to merge 8 commits into
Open
feat(bug-finding): warn users if resource config conflicts with machi…#159tkqdldk wants to merge 8 commits into
tkqdldk wants to merge 8 commits into
Conversation
azchin
requested changes
Mar 30, 2026
azchin
left a comment
azchin
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the PR and sorry for the late response! I have a few requests detailed below. We also use a few linters and formatters in our CI workflow, so I recommend running them manually before pushing.
| except(OSError): | ||
| pass | ||
|
|
||
| memeinfo = Path("/proc/meminfo") |
Collaborator
There was a problem hiding this comment.
meminfo typo (caught by ruff)
| machine_cpu_count = os.cpu_count() | ||
| machine_memory = get_host_memory() | ||
|
|
||
| if machine_cpu_count is None or machine_memory == 0: |
Collaborator
There was a problem hiding this comment.
(C1) Let's get machine_memory to return None on failure
| except subprocess.CalledProcessError as e: | ||
| raise RuntimeError(f"Error removing {path} with Docker: {e}") | ||
|
|
||
| def get_host_memory() -> int: |
Collaborator
There was a problem hiding this comment.
Return val as int | None, see (C1)
| for line in meminfo.read_text().splitlines(): | ||
| if line.startswith("MemTotal:"): | ||
| return int(line.split()[1])*1024 | ||
| return 0 |
| except ValueError as e: | ||
| log_warning(f"Failed to parse memory for {name}: {e}") | ||
|
|
||
| cpu_check = max_cpus_required <= machine_cpu_count |
Collaborator
There was a problem hiding this comment.
I think this is an off-by-one, since os.cpu_count() returns number of CPUs but cpuset is indexed by zero, so we'd get range of 0-3 on 4-core machine. max([0, 1, 2, 3]) == 3 < 4
max_cpus_required < machine_cpu_count would be the correct check. But please check/test for this edge case dynamically.
Author
There was a problem hiding this comment.
You're absolutely right ^^'... I will also add dynamic testing.
tkqdldk
force-pushed
the
feat/resource-config-warn
branch
2 times, most recently
from
867fac0 to
c55125d
Compare
Collaborator
|
Hm I'm not seeing the error messages implemented in your PR in my output. The build does indeed abort, but that's because of Docker's error handling and happens after I can take a deeper look at what we're missing after the weekend, if needed. |
Author
Hi, sorry for the late response, so the validation only runs before run currently, as I understood that would be the only need. I'll extend the check to build-target so the warning shows here too. |
Collaborator
|
Hi, sorry for the delayed feedback as we were working on a big internal refactor. I'll take a proper look this weekend, in the meanwhile could you check the CI failure? In |
tkqdldk
force-pushed
the
feat/resource-config-warn
branch
from
c55125d to
dca9899
Compare
Author
|
No worries ^^ Resolved the CI failure on my part and added the check in the build-target step. Let me know if I need to add or adjust anything once you take a look. |
azchin
requested changes
Apr 20, 2026
azchin
left a comment
azchin
left a comment
Collaborator
There was a problem hiding this comment.
I'd like the check to be placed earlier, and error + terminate.
| resolved_source_path = source_dir | ||
|
|
||
| tasks = [] | ||
| tasks: list[tuple[str, Callable[[MultiTaskProgress], TaskResult]]] = [ |
Collaborator
There was a problem hiding this comment.
Could you call _validate_machine_resources near the start of build(), before the target.build_docker_image() gets run? Don't need to adhere to the tasks loop if calling it directly makes it easier.
| memory_check = total_memory_required <= machine_memory | ||
|
|
||
| if not cpu_check or not memory_check: | ||
| log_warning( |
Collaborator
There was a problem hiding this comment.
Let's produce an error instead of warning, and terminate early. Current behavior is I see the warning message, but the docker compose still tries to run.
Collaborator
|
CI failed b/c of LLM API key not being passed correctly, I'll look into that. |
tkqdldk
force-pushed
the
feat/resource-config-warn
branch
from
5aa53b2 to
38db6c4
Compare
Author
|
Hi, sorry for the delay, I was very busy lately. Added the changes according to your comments. |
azchin
approved these changes
May 8, 2026
Collaborator
|
Looks good to me! There's an integration test failure because it looks like we're overallocating memory somehow.
+++ b/oss_crs/tests/integration/test_builder_sidecar_full.py
@@ -33,11 +33,11 @@ def sidecar_full_compose(tmp_dir):
content = {
"run_env": "local",
"docker_registry": "local",
- "oss_crs_infra": {"cpuset": "0-3", "memory": "8G"},
+ "oss_crs_infra": {"cpuset": "0-3", "memory": "7G"},
"builder-sidecar-full": {
"source": {"local_path": str(CRS_FIXTURE)},
"cpuset": "0-3",
- "memory": "8G",
+ "memory": "7G",
},
}
path = tmp_dir / "compose.yaml"
diff --git a/oss_crs/tests/integration/test_builder_sidecar_lite.py b/oss_crs/tests/integration/test_builder_sidecar_lite.py
index 9c71c17..237923b 100644
--- a/oss_crs/tests/integration/test_builder_sidecar_lite.py
+++ b/oss_crs/tests/integration/test_builder_sidecar_lite.py
@@ -36,11 +36,11 @@ def sidecar_lite_compose(tmp_dir):
content = {
"run_env": "local",
"docker_registry": "local",
- "oss_crs_infra": {"cpuset": "0-3", "memory": "8G"},
+ "oss_crs_infra": {"cpuset": "0-3", "memory": "7G"},
"builder-sidecar-lite": {
"source": {"local_path": str(CRS_FIXTURE)},
"cpuset": "0-3",
- "memory": "8G",
+ "memory": "7G",
},
}
path = tmp_dir / "compose.yaml" |
Collaborator
|
Hello @tkqdldk , are you going to continue working on this PR? If you no longer have interest or bandwidth, I'll close the PR and integrate your contributions in another branch + PR so that I can make some modifications. |
Author
Hi, sorry again for the delay. It seemed to be a rounding error from integer division the last time I checked. I can still fix it if necessary or if you choose so. |
Collaborator
|
You can go ahead and fix it. Just wanted to check up on this |
Collaborator
|
Could you check the pyright error? I'll merge as soon as the CI passes, your code looks good. |
Author
|
Hi, I looked through the pyright error outputs and none seem related to the resource check nor the error/termination which follow the failing of the check. |
Collaborator
|
Weird, upstream doesn't have pyright errors. I'll merge main into your branch |
azchin
reviewed
Jun 2, 2026
| for name, crs_cfg in self.config.crs_entries.items() | ||
| ] | ||
|
|
||
| builder_count = sum(1 for crs in self.crs_list if crs.config.is_builder) |
Collaborator
There was a problem hiding this comment.
I think pyright is complaining about this line? is_builder
…ne resources Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
… memory output Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
… to integer division Signed-off-by: tkqdldk <samia.20.bouchaal@gmail.com>
tkqdldk
force-pushed
the
feat/resource-config-warn
branch
from
76aa1e2 to
44f6826
Compare
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
2 participants
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.
Closes #49
Description
Adds resources check and a warning when the compose file resource configurations (max CPU and total memory) exceed machine resources. The check runs in "Validate Configuration for Running" phase, before Docker attempts to start containers.
The function emits a warning but does not stop the execution.
Changes
get_host_total_memory()inutils.py_validate_machine_resources()incrs_compose.py__validate_before_runValidation
All existing tests pass.
P.S. : Can't assign a reviewer directly, cc @azchin for review