🌱 add samples and docs of the Dynamic Scoring Framework - #106
Conversation
Signed-off-by: Kazuma Takeuchi <kazuma.takeuchi02@g.softbank.co.jp>
There was a problem hiding this comment.
Can the doc setup-local-clusters.md be just replaced by the command provided by https://open-cluster-management.io/docs/getting-started/quick-start/? so that you don't need to maintain the ocm setup doc in this repo.
curl -L https://raw.githubusercontent.com/open-cluster-management-io/OCM/main/solutions/setup-dev-environment/local-up.sh | bash
There was a problem hiding this comment.
Sorry for the delay.
I referred to the OCM Quickstart and replaced the names as follows:
hub01 → hub
worker01 → cluster1
worker02 → cluster2
qiujian16
left a comment
qiujian16
left a comment
There was a problem hiding this comment.
Thanks. I have left some comments.
| print("Preference Scores:", preference_scores) | ||
| print("Resource Capacity:", resource_capacity) | ||
|
|
||
| # --- モデル定義 ------------------------------------------------- |
There was a problem hiding this comment.
could we convert this to English?
| @@ -0,0 +1,140 @@ | |||
| from fastapi import FastAPI, HTTPException | |||
There was a problem hiding this comment.
I did not see a readme under this directory. And I think there should also be a deps configuration file to install all libs?
There was a problem hiding this comment.
I had assumed Docker, so I wrote it directly in the Dockerfile.
Since dependencies should be organized in a separate file, I’ll fix that.
I’ll also add a README for each sample.
| @@ -0,0 +1,14564 @@ | |||
| { | |||
| "data": [ | |||
There was a problem hiding this comment.
what this file used for? Is it just an output example? I see several repo has this file, and it is hard to read. Is it necessary to maintain them?
There was a problem hiding this comment.
This is example data for testing the Scoring API. It represents input from the DSF Agent to the Scoring API.
We could omit it, but we sometimes need test data to validate the Scoring API.
How about renaming sample_cpu_load.json to testdata_cpu_load.json?
Signed-off-by: Kazuma Takeuchi <kazuma.takeuchi02@g.softbank.co.jp>
Signed-off-by: Kazuma Takeuchi <kazuma.takeuchi02@g.softbank.co.jp>
WalkthroughAdds many new sample services, manifests, and large documentation/spec updates; renames cluster/context identifiers; updates Dockerfile build args and Makefile build flags; adds OpenShift BuildConfigs; introduces a policy-watcher daemon and an MCP optimizer service; and adds multiple FastAPI scorer implementations with sample data and deployment manifests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| .PHONY: docker-build-controller | ||
| docker-build-controller: ## Build docker image with the manager. | ||
| $(CONTAINER_TOOL) build -t ${IMG_CONTROLLER} . | ||
| $(CONTAINER_TOOL) build $(IMAGE_BUILD_EXTRA_FLAGS) -t ${IMG_CONTROLLER} . |
There was a problem hiding this comment.
For arm64 image build.
| .PHONY: docker-build-addon | ||
| docker-build-addon: ## Build docker image with the addon. | ||
| $(CONTAINER_TOOL) build -t ${IMG_ADDON} . -f Dockerfile.addon --no-cache | ||
| $(CONTAINER_TOOL) build $(IMAGE_BUILD_EXTRA_FLAGS) -t ${IMG_ADDON} . -f Dockerfile.addon --no-cache |
There was a problem hiding this comment.
For arm64 image build.
| # the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore, | ||
| # by leaving it empty we can ensure that the container and binary shipped on it will have the same platform. | ||
| RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o manager cmd/main.go | ||
| RUN CGO_ENABLED=0 GOOS=${OS} GOARCH=${ARCH} go build -a -o manager cmd/main.go |
There was a problem hiding this comment.
For arm64 image build.
| # the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore, | ||
| # by leaving it empty we can ensure that the container and binary shipped on it will have the same platform. | ||
| RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o dynamic-scoring-addon cmd/main.go | ||
| RUN CGO_ENABLED=0 GOOS=${OS} GOARCH=${ARCH} go build -a -o dynamic-scoring-addon cmd/main.go |
There was a problem hiding this comment.
For arm64 image build.
|
|
||
| Please refer to the [quick start guide](docs/quickstart.md) for instructions on deploying the Dynamic Scoring Framework in your environment. | ||
|
|
||
| ## Further Reading |
There was a problem hiding this comment.
I have added guidance to the Scoring API sample collection and included an example of resource allocation optimization using DSF.
Signed-off-by: Kazuma Takeuchi <kazuma.takeuchi02@g.softbank.co.jp>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dynamic-scoring-framework/Dockerfile.addon (1)
20-23:⚠️ Potential issue | 🟡 MinorUpdate stale GOARCH comment.
Line 20 says GOARCH has no default, but Line 4 sets
ARG ARCH=amd64. Please align the comment with actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/Dockerfile.addon` around lines 20 - 23, The comment about GOARCH is out of date: the Dockerfile sets ARG ARCH=amd64 (and thus provides a default), so update the explanatory comment to reflect that ARCH/GOARCH can be overridden but has a default of amd64; specifically, edit the commentary that references GOARCH to mention ARG ARCH=amd64 (and describe that leaving it empty is optional because the ARG defaults to amd64, while still allowing override to match host via BUILDPLATFORM/arch), and ensure references to variables use the exact identifiers GOARCH and ARG ARCH so reviewers can find and verify the change.dynamic-scoring-framework/docs/development.md (1)
13-13:⚠️ Potential issue | 🟠 MajorUpdate Go version to a currently supported release.
Go 1.24 is outdated. The latest stable version is Go 1.26.1. Use
golang:1.26or at minimumgolang:1.25to ensure the documentation reflects a supported, maintained version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/docs/development.md` at line 13, The documented base image tag "golang:1.24" is out of date; update the reference in the docs (the string "golang:1.24") to a currently supported release such as "golang:1.26" (or at minimum "golang:1.25") so the development.md reflects a maintained Go version.
🟠 Major comments (23)
dynamic-scoring-framework/samples/simple-prediction-scorer/app/get_metrics.py-23-45 (1)
23-45:⚠️ Potential issue | 🟠 MajorAdd timeout and request error handling for the Prometheus call.
Line 23 has a
requests.get()call without timeout, and network errors are not handled before JSON parsing. This can cause the script to hang indefinitely on connection issues or fail with unhandled exceptions on transient errors.Suggested fix
-response = requests.get(PROM_URL, params=params) -data = response.json() +try: + response = requests.get(PROM_URL, params=params, timeout=10) + response.raise_for_status() + data = response.json() +except (requests.RequestException, ValueError) as exc: + print(f"Error querying Prometheus: {exc}") + raise SystemExit(1) -if data["status"] == "success": +if data.get("status") == "success":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/simple-prediction-scorer/app/get_metrics.py` around lines 23 - 45, The Prometheus call using requests.get(PROM_URL, params=params) lacks a timeout and error handling; wrap the HTTP call and JSON parsing in a try/except: call requests.get with a sensible timeout (e.g., timeout=10), call response.raise_for_status() to surface non-2xx responses, catch requests.exceptions.RequestException to handle network/timeouts and print/log the error, then catch JSONDecodeError/ValueError around response.json() and handle it similarly; only proceed to access data["status"] and write sample_cpu_load.json after successful response.json() parsing and status checks. Ensure you reference the same symbols (requests.get, PROM_URL, params, response.json, sample_cpu_load.json) and exit/return early on errors to avoid further exceptions.dynamic-scoring-framework/samples/ai-workload-scorer/Dockerfile-1-16 (1)
1-16:⚠️ Potential issue | 🟠 MajorAdd a non-root USER directive.
The container runs as root, which violates container security best practices. This is especially important given the world-writable
/app/modelsdirectory.See the proposed fix in the previous comment, which addresses both the permissions and user issues together.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/Dockerfile` around lines 1 - 16, Replace the root-only setup and world-writable /app/models with a non-root runtime user: create a dedicated user/group (e.g., appuser), chown the application directories including /app/models to that user, remove or avoid chmod 777, and add a USER appuser directive before EXPOSE/CMD so the container runs as the non-root user; update the Dockerfile steps around RUN mkdir -p /app/models and RUN chmod -R 777 /app/models to instead create the directory, chown it to appuser (and set conservative permissions like 755), then switch to USER appuser so CMD ["python", "main.py"] executes as the non-root account.dynamic-scoring-framework/samples/static-scorer/Dockerfile-1-11 (1)
1-11:⚠️ Potential issue | 🟠 MajorAdd a non-root USER directive.
The container runs as root, which violates container security best practices. Specify a non-root user to reduce the attack surface.
🔒 Proposed fix to run as non-root user
RUN pip install --no-cache-dir fastapi uvicorn pydantic EXPOSE 8000 +# Create and switch to non-root user +RUN useradd -m -u 1000 appuser && \ + chown -R appuser:appuser /app +USER appuser + CMD ["python", "main.py"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/static-scorer/Dockerfile` around lines 1 - 11, The Dockerfile currently runs as root; create and switch to a non-root user by adding steps to create a user/group (e.g., appuser), chown the WORKDIR files to that user, and add a USER appuser directive before the CMD; update the COPY/WORKDIR/RUN sequence around COPY app ./, ensure ownership is changed (chown) for /app and any installed artifacts, and place USER appuser after RUN pip install and before CMD to ensure the container runs processes as the non-root user.dynamic-scoring-framework/samples/llm-forecast-scorer/Dockerfile-1-11 (1)
1-11:⚠️ Potential issue | 🟠 MajorAdd a non-root USER directive.
The container runs as root, which violates container security best practices. Specify a non-root user to reduce the attack surface.
🔒 Proposed fix to run as non-root user
RUN pip install --no-cache-dir fastapi uvicorn pydantic requests jinja2 EXPOSE 8000 +# Create and switch to non-root user +RUN useradd -m -u 1000 appuser && \ + chown -R appuser:appuser /app +USER appuser + CMD ["python", "main.py"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/llm-forecast-scorer/Dockerfile` around lines 1 - 11, The Dockerfile currently runs as root; update it to create and switch to a non-root user (e.g., add a system user like "appuser"), chown the WORKDIR contents after COPY, and set USER appuser before CMD so the container runs unprivileged; specifically modify the Dockerfile around WORKDIR /app, COPY app ./ and CMD ["python","main.py"] to add a user/group creation (no-login/non-root), change ownership of /app (chown -R appuser:appuser /app), and add USER appuser to ensure the process runs as that non-root user.dynamic-scoring-framework/samples/policy-watcher/Dockerfile-1-20 (1)
1-20:⚠️ Potential issue | 🟠 MajorAdd a non-root USER directive.
The container runs as root, which violates container security best practices. Specify a non-root user to reduce the attack surface if the container is compromised.
🔒 Proposed fix to run as non-root user
# Ensure the virtual environment is in PATH ENV PATH="/app/.venv/bin:$PATH" +# Create and switch to non-root user +RUN useradd -m -u 1000 appuser && \ + chown -R appuser:appuser /app +USER appuser CMD ["python", "-u", "main.py"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/policy-watcher/Dockerfile` around lines 1 - 20, Add a non-root user and run the container as that user: create a dedicated user (e.g., "appuser") and group after setting WORKDIR /app, chown the app directory to that user before switching, then add a USER appuser directive so subsequent RUN/CMD execute as non-root; ensure the virtualenv path (ENV PATH="/app/.venv/bin:$PATH") remains accessible by the new user and any files created by RUN uv venv and RUN uv sync are owned by that user.dynamic-scoring-framework/samples/ai-workload-scorer/Dockerfile-9-10 (1)
9-10:⚠️ Potential issue | 🟠 MajorAvoid world-writable permissions (777).
Setting
chmod 777on/app/modelsgrants read, write, and execute permissions to all users, creating a significant security risk. If the directory needs to be writable by the application, use more restrictive permissions (e.g.,755) or proper user ownership.🔒 Proposed fix with safer permissions
RUN mkdir -p /app/models -RUN chmod -R 777 /app/models + +# Create non-root user and set ownership +RUN useradd -m -u 1000 appuser && \ + chown -R appuser:appuser /app +USER appuserThis approach gives the application user full control while preventing other users from accessing the directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/Dockerfile` around lines 9 - 10, The Dockerfile currently makes /app/models world-writable via the RUN chmod -R 777 /app/models, which is unsafe; change this to set restrictive permissions and proper ownership by replacing the chmod 777 step with a safer pattern: create /app/models, chown it to the non-root app user (or use USER in the image) and set permissions to e.g. 755 (or 750) so the application user can read/write while others cannot; update the RUN commands that reference /app/models (the mkdir and chmod lines) to perform chown to the app user and set 755/750 instead of 777.dynamic-scoring-framework/samples/simple-prediction-scorer/Dockerfile-1-11 (1)
1-11:⚠️ Potential issue | 🟠 MajorRun the container as a non-root user.
Line 1–Line 11 currently run the app as root, which weakens container isolation.
🔐 Proposed hardening diff
FROM python:3.11-slim WORKDIR /app +RUN addgroup --system app && adduser --system --ingroup app app + -COPY app ./ +COPY --chown=app:app app ./ RUN pip install --no-cache-dir fastapi uvicorn pydantic darts EXPOSE 8000 +USER app + CMD ["python", "main.py"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/simple-prediction-scorer/Dockerfile` around lines 1 - 11, The Dockerfile currently runs the container as root; add a non-root user and switch to it to harden isolation: create a user (e.g., "appuser") and group, chown the WORKDIR and copied files (referenced by WORKDIR /app and COPY app ./), then add a USER appuser directive before EXPOSE/CMD so the container process (CMD ["python", "main.py"]) runs unprivileged; ensure any package installation that requires root remains in earlier RUN steps and file ownership is updated accordingly.dynamic-scoring-framework/samples/simple-prediction-scorer/manifests/simple-prediction-scorer.yaml-22-23 (1)
22-23:⚠️ Potential issue | 🟠 MajorUpdate image reference to match the simple-prediction-scorer sample.
The manifest uses
sample-scorerimage, but should usesimple-prediction-scorer. This appears throughout: the deployment name (line 4), service address (line 7), pod labels (lines 14, 18), container name (line 21), and image reference (line 22). The correct image isquay.io/dynamic-scoring/simple-prediction-scorer:latest, as documented in the README and implemented inmanifestwork.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/simple-prediction-scorer/manifests/simple-prediction-scorer.yaml` around lines 22 - 23, Replace all occurrences of "sample-scorer" with "simple-prediction-scorer" in this manifest: update the Deployment name (currently using sample-scorer), the Service address, the pod labels (both label keys/values referencing sample-scorer), the container name in the Pod spec, and the image reference (change quay.io/dynamic-scoring/sample-scorer:latest to quay.io/dynamic-scoring/simple-prediction-scorer:latest) so the manifest matches the simple-prediction-scorer sample and the README/manifestwork.yaml.dynamic-scoring-framework/samples/simple-prediction-scorer/manifests/simple-prediction-scorer.yaml-19-25 (1)
19-25:⚠️ Potential issue | 🟠 MajorHarden pod/container security context.
Line 19–Line 25 use default security settings; add explicit non-root and privilege restrictions.
🛡️ Proposed securityContext hardening
spec: replicas: 1 selector: @@ template: metadata: labels: app: sample-scorer spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: sample-scorer image: quay.io/dynamic-scoring/sample-scorer:latest imagePullPolicy: Never + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL ports: - containerPort: 8000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/simple-prediction-scorer/manifests/simple-prediction-scorer.yaml` around lines 19 - 25, The container spec for "sample-scorer" lacks hardened securityContext; update the pod/container spec (spec.containers -> name: sample-scorer) to enforce non-root execution and restrict privileges by adding a securityContext with runAsNonRoot: true, a specific runAsUser (e.g., 1000), allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, capabilities: drop: ["ALL"], privileged: false, and a seccompProfile (runtime/default); apply these settings either at the pod spec level or the container (sample-scorer) securityContext to ensure the container cannot run as root or escalate privileges.dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/Dockerfile-1-23 (1)
1-23:⚠️ Potential issue | 🟠 MajorAdd a non-root runtime user in this image.
Line 1–Line 23 currently execute as root; this should be hardened before release.
🔐 Proposed hardening diff
FROM python:3.12-slim WORKDIR /app # Install uv RUN pip install --no-cache-dir uv +RUN addgroup --system app && adduser --system --ingroup app app # Copy project files -COPY main.py . -COPY pyproject.toml . -COPY manifests ./manifests -COPY templates ./templates +COPY --chown=app:app main.py . +COPY --chown=app:app pyproject.toml . +COPY --chown=app:app manifests ./manifests +COPY --chown=app:app templates ./templates # Create virtual environment and install dependencies RUN uv venv && \ uv sync # Ensure the virtual environment is in PATH ENV PATH="/app/.venv/bin:$PATH" EXPOSE 8338 +USER app + CMD ["python", "main.py"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/Dockerfile` around lines 1 - 23, The Dockerfile currently runs as root; create and switch to a non-root runtime user (e.g., add a dedicated user/group and set ownership of /app) before the final image runs so processes invoked by CMD ["python", "main.py"] execute unprivileged. Update the build steps around the virtualenv creation (RUN uv venv && uv sync) to chown the created files to that user, set ENV HOME appropriately, and add a USER <username> directive so the PATH line (ENV PATH="/app/.venv/bin:$PATH") and the final CMD run as the non-root user.dynamic-scoring-framework/samples/llm-forecast-scorer/app/contexts/tendency.txt.org-1-5 (1)
1-5:⚠️ Potential issue | 🟠 MajorUpdate
main.py:80to load the new file with.orgextension.The code currently loads
./contexts/tendency.txt, but the new file istendency.txt.org. Update line 80 from:with open("./contexts/tendency.txt", "r") as f:to:
with open("./contexts/tendency.txt.org", "r") as f:Otherwise, the new file will never be loaded and the system will continue using the old content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/llm-forecast-scorer/app/contexts/tendency.txt.org` around lines 1 - 5, The file-open path in main.py (around the code that opens the tendency context at line 80) is still pointing to "./contexts/tendency.txt" so the new file tendency.txt.org is never loaded; update the string passed to open in that block (the with open(...) call in main.py) to "./contexts/tendency.txt.org" so the new .org file is read instead of the old .txt file.dynamic-scoring-framework/samples/simple-prediction-scorer/manifests/cpu-load-generator.yaml-15-32 (1)
15-32:⚠️ Potential issue | 🟠 MajorHarden pod/container security context before shipping this sample.
The workload currently runs with default privileges; that permits root/default escalation paths. Please set explicit security controls at pod and container level.
🔒 Suggested hardening patch
spec: + template: + spec: + securityContext: + seccompProfile: + type: RuntimeDefault containers: - name: stressor image: polinux/stress + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/simple-prediction-scorer/manifests/cpu-load-generator.yaml` around lines 15 - 32, The pod currently runs with default privileges; harden both pod and container security contexts by adding a pod-level spec.securityContext (e.g., runAsNonRoot: true, runAsUser: <non-root uid>, runAsGroup: <non-root gid>, fsGroup: <gid>) and a container-level securityContext inside the container named "stressor" (set allowPrivilegeEscalation: false, privileged: false, readOnlyRootFilesystem: true, capabilities: drop: ["ALL"], seccompProfile: {type: "RuntimeDefault"}), and ensure resource usage and requests remain intact; update the manifest to include these securityContext entries so the "stressor" container no longer runs as root or with elevated capabilities.dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/clustersetbindings.yaml-5-5 (1)
5-5:⚠️ Potential issue | 🟠 MajorUpdate
ManagedClusterSetBindingnamespace to match sample deployment namespace.The hardcoded
defaultnamespace will break placements when the sample is deployed to a different namespace. Since the templates use parameterized{{namespace}}and the example output showsmy-namespace, the binding must be in the same namespace as the generated Placements to be inherited.Suggested fix (for example, if deploying to `my-namespace`)
metadata: name: global - namespace: default + namespace: my-namespace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/clustersetbindings.yaml` at line 5, The ManagedClusterSetBinding resource currently has a hardcoded namespace "default" which will break placements; update the namespace field of the ManagedClusterSetBinding in clustersetbindings.yaml to match the deployment namespace used by the templates (use the parameterized value {{namespace}} or the example namespace "my-namespace") so it aligns with the generated Placement resources and inherits correctly.dynamic-scoring-framework/Dockerfile.addon-3-4 (1)
3-4:⚠️ Potential issue | 🟠 MajorDockerfile.addon should use TARGETOS/TARGETARCH for proper multi-platform build support.
The custom
OSandARCHARG names will default tolinux/amd64(lines 3-4) since the build command doesn't pass these args explicitly. Standard Docker/buildx practice is to useTARGETOSandTARGETARCH, which are automatically populated by buildx in multi-platform builds. This makes the addon build consistent with the controller's buildx pattern in the Makefile (line 242) and enables proper architecture propagation when usingdocker buildx.✅ Recommended change
-ARG OS=linux -ARG ARCH=amd64 +ARG TARGETOS=linux +ARG TARGETARCH=amd64-RUN CGO_ENABLED=0 GOOS=${OS} GOARCH=${ARCH} go build -a -o dynamic-scoring-addon cmd/main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -a -o dynamic-scoring-addon cmd/main.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/Dockerfile.addon` around lines 3 - 4, The Dockerfile.addon uses custom build args ARG OS and ARG ARCH which won't be populated by buildx; replace those with the standard buildx-provided ARG TARGETOS and ARG TARGETARCH (and any downstream references to OS/ARCH within this file) so multi-platform builds inherit architecture/OS from buildx; update occurrences of OS/ARCH to TARGETOS/TARGETARCH and ensure any ENV or COPY/GOOS/GOARCH uses reference the new ARG names (e.g., TARGETOS, TARGETARCH) to match the controller's buildx pattern.dynamic-scoring-framework/samples/static-scorer/manifests/manifestwork.yaml-30-31 (1)
30-31:⚠️ Potential issue | 🟠 MajorImage pull settings are likely to break sample deployment on real clusters.
imagePullPolicy: Neverwith a remote image reference will fail unless every target node preloads that exact image;latestalso hurts reproducibility.Suggested fix
- image: quay.io/dynamic-scoring/static-scorer:latest - imagePullPolicy: Never + image: quay.io/dynamic-scoring/static-scorer:<pinned-tag> + imagePullPolicy: IfNotPresent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/static-scorer/manifests/manifestwork.yaml` around lines 30 - 31, The manifest currently uses image: quay.io/dynamic-scoring/static-scorer:latest and imagePullPolicy: Never which will fail on real clusters; update the image reference to a fixed, versioned tag (not "latest") and set imagePullPolicy to IfNotPresent (or Always if you want to force refresh) so kubelets will pull the remote image; modify the image line (quay.io/dynamic-scoring/static-scorer:...) and the imagePullPolicy field to apply this change.dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-disable-mig-cluster2.yaml-17-17 (1)
17-17:⚠️ Potential issue | 🟠 MajorUse unique
ConfigurationPolicynames to avoid collisions across policy manifests.These identical names (
enforce-miglabel,check-migresultlabel) appear in all 6 policy sample files and will overwrite each other if deployed to the same namespace. Rename each to include the policy variant (cluster/enable/disable) for uniqueness.Suggested fix
- name: enforce-miglabel + name: disable-mig-cluster2-enforce-miglabel ... - name: check-migresultlabel + name: disable-mig-cluster2-check-migresultlabelAlso applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-disable-mig-cluster2.yaml` at line 17, The ConfigurationPolicy names "enforce-miglabel" and "check-migresultlabel" are duplicated across sample manifests; update the metadata.name fields in these policies (ConfigurationPolicy) to be unique per variant by appending the variant identifier (e.g., "-cluster2", "-disable", "-enable") so each file has distinct names like "enforce-miglabel-disable" and "check-migresultlabel-disable" (or similar consistent naming) to avoid overwrite collisions when deployed to the same namespace.dynamic-scoring-framework/samples/policy-watcher/deployment.yaml-48-63 (1)
48-63:⚠️ Potential issue | 🟠 MajorHarden container and pod security context defaults.
The deployment currently runs with default security settings, which permits avoidable privilege risk.
🔐 Suggested hardening
spec: replicas: 1 @@ template: @@ spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault serviceAccountName: policy-watcher-sa imagePullSecrets: - name: hub-registry-secret containers: - name: policy-watcher image: quay.io/dynamic-scoring/policy-watcher:v0.1.0 imagePullPolicy: IfNotPresent + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] env:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/policy-watcher/deployment.yaml` around lines 48 - 63, The Deployment for the "policy-watcher" container (container name policy-watcher, serviceAccountName policy-watcher-sa) lacks hardened securityContext; add a pod-level securityContext with runAsNonRoot: true, runAsUser: 1000 (or appropriate UID), and fsGroup: 2000, and on the container spec add securityContext with allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, runAsNonRoot: true, runAsUser: 1000, capabilities.drop: ["ALL"], and seccompProfile.type: RuntimeDefault to prevent privilege escalation and enforce non-root execution for the policy-watcher container and its pods.dynamic-scoring-framework/samples/llm-forecast-scorer/app/llm-test.py-23-24 (1)
23-24:⚠️ Potential issue | 🟠 MajorAdd explicit timeout to prevent indefinite blocking on stalled inference endpoint.
Line 23 can hang indefinitely if the vLLM endpoint becomes unresponsive. The
requests.post()call has no timeout, which is a reliability hazard in production.⏱️ Suggested fix
- response = requests.post(VLLM_ENDPOINT, json=payload) + response = requests.post(VLLM_ENDPOINT, json=payload, timeout=30)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/llm-forecast-scorer/app/llm-test.py` around lines 23 - 24, The requests.post call to VLLM_ENDPOINT using payload can hang indefinitely; add an explicit timeout to requests.post (e.g., requests.post(VLLM_ENDPOINT, json=payload, timeout=...)) and make the timeout configurable via a constant or env var (e.g., VLLM_TIMEOUT) so callers can adjust; ensure any Timeout/RequestException from requests is either propagated or handled consistently with the surrounding error handling near response.raise_for_status.dynamic-scoring-framework/samples/llm-forecast-scorer/app/main.py-54-56 (1)
54-56:⚠️ Potential issue | 🟠 MajorAdd timeout to
requests.postto prevent indefinite hangs.The HTTP call to the inference endpoint lacks a timeout, which can cause the service to hang indefinitely if the LLM backend is unresponsive.
🐛 Proposed fix
- response = requests.post(INFERENCE_ENDPOINT, json=payload) + response = requests.post(INFERENCE_ENDPOINT, json=payload, timeout=60)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/llm-forecast-scorer/app/main.py` around lines 54 - 56, The call to requests.post(INFERENCE_ENDPOINT, json=payload) can hang without a timeout; update the request in main.py (the requests.post call that assigns to response) to include a reasonable timeout argument (e.g., timeout=...) and wrap the call in a try/except that catches requests.Timeout and requests.RequestException, converting failures into an HTTPException with a 500 (or appropriate) status and the error message so the service fails fast instead of hanging.dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/deployment.yaml-58-65 (1)
58-65:⚠️ Potential issue | 🟠 MajorAdd security context to harden the container.
The container lacks security hardening. Per static analysis (Trivy/Checkov), add a
securityContextto prevent privilege escalation, enforce read-only root filesystem, and run as non-root.🛡️ Proposed fix
spec: serviceAccountName: dynamic-scoring-framework-mcp-sa containers: - name: dynamic-scoring-framework-mcp image: quay.io/dynamic-scoring/dynamic-scoring-framework-mcp:latest imagePullPolicy: Never ports: - containerPort: 8338 + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsNonRoot: true + capabilities: + drop: + - ALL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/deployment.yaml` around lines 58 - 65, Add a securityContext to the container spec for the container named "dynamic-scoring-framework-mcp" to harden it: set securityContext.allowPrivilegeEscalation=false, securityContext.readOnlyRootFilesystem=true, securityContext.runAsNonRoot=true (and set runAsUser to a non-root UID, e.g., 1000), and drop all capabilities (securityContext.capabilities.drop: ["ALL"]); you can also add a pod-level securityContext with fsGroup/runAsUser if needed to ensure file ownership consistency. Ensure this block is nested under the same container entry that contains image/imagePullPolicy so the runtime applies these restrictions to the dynamic-scoring-framework-mcp container.dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py-131-176 (1)
131-176:⚠️ Potential issue | 🟠 MajorLines 131-176: Validate
Hostnamebefore mutatingdfs_node.
dim_relevant_colsis only initialized for non-null hostnames, but this block still insertsdfs_node["unknown"]before the Line 171 lookup can fail. That exception is swallowed, the partial node stays indfs_node, and the laterdim_relevant_cols[node_label][dim]access can turn a single bad series into a 500.Suggested fix
- node_label = series.metric.get("Hostname", "unknown") + node_label = series.metric.get("Hostname") gpu_id = series.metric.get("GPU_I_ID", "0") gpu_device = resolve_device_profile(series.metric.get("GPU_I_PROFILE", "all")) pod_label = series.metric.get("exported_pod", "unknown") app_name = resolve_application_name(pod_label) + if not node_label: + print("Skip series due to missing Hostname label.") + continue + dim_relevant_cols.setdefault(node_label, {dim: [] for dim in dimentions}) + value_label = f"value_{node_label}_GPU{gpu_id}_{gpu_device}_{pod_label}" values = [float(v[1]) for v in series.values] times = [v[0] for v in series.values]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around lines 131 - 176, The loop can create dfs_node entries for an "unknown" or uninitialized Hostname and leave partial state if processing fails; ensure you validate node_label is in the pre-initialized dim_relevant_cols before mutating dfs_node or adding dims. Specifically, in the loop handling series (use node_label, pod_label, values, times), skip any series whose node_label is not present in dim_relevant_cols (or is "unknown") before constructing/merging DataFrames; build/merge into a temporary df and only assign to dfs_node[node_label] after merge succeeds, and when appending to dim_relevant_cols[node_label][(app_name, gpu_device)] ensure that node_label and the (app_name,gpu_device) key exist to avoid KeyError.dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py-119-120 (1)
119-120:⚠️ Potential issue | 🟠 MajorLines 119-120: Convert to sync handler to avoid blocking the event loop.
This async handler contains no
awaitstatements but performs CPU-heavy Pandas/NumPy/Darts operations plus model file I/O. In FastAPI, this blocks the event loop thread, preventing other requests from being processed concurrently on the same worker. Convert to a sync function so FastAPI runs it in a thread pool instead.Suggested fix
`@app.post`("/power/scoring", response_model=ScoringResponse) -async def power_scoring_timeseries(payload: ScoringPayload, request: Request): +def power_scoring_timeseries(payload: ScoringPayload, request: Request):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around lines 119 - 120, The handler power_scoring_timeseries is defined async but contains no awaits and runs CPU-heavy Pandas/NumPy/Darts work plus model file I/O, which will block the event loop; change it to a synchronous function (remove async) so FastAPI will run it in a worker thread, and ensure any internal blocking I/O (model loading, file reads) remains synchronous or is offloaded where appropriate; update the function signature (power_scoring_timeseries(payload: ScoringPayload, request: Request)) and any callers or decorators if needed to reflect the sync handler.dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py-211-243 (1)
211-243:⚠️ Potential issue | 🟠 MajorLines 231–240: Handle model training failures to avoid unfitted model prediction.
When
fit()orsave()fails, the code catches the exception and logs it, then immediately callspredict()on line 240. The problem:LinearRegressionModel.predict()raises aValueErrorif the model has not been fit. This happens in two scenarios:
- No cached model: A new unfitted instance is created, trained, and on failure, predict is called on an unfitted model.
- Cached model exists: The loaded model is overwritten at line 231 with a new unfitted instance, and on training failure, predict is called on that unfitted instance.
Both paths produce a hard failure instead of gracefully handling the transient training error.
Preserve the previously loaded model on training failure, or provide a fallback value if no model is available:
Suggested fix
model_path = os.path.join(MODEL_DIR, f"{node_label}.pth") + model = None + ts = TimeSeries.from_dataframe( + df_node, "timestamp", "total_power", fill_missing_dates=True, freq="60s" + ) if os.path.exists(model_path): model = LinearRegressionModel.load(model_path) print(f"Loaded model for {node_label} from {model_path}") - ts = TimeSeries.from_dataframe( - df_node, "timestamp", "total_power", fill_missing_dates=True, freq="60s" - ) train, val = ts[:-HORIZON_LENGTH], ts[-HORIZON_LENGTH:] pred = model.predict(n=HORIZON_LENGTH, series=train) true_vals = val.values().flatten() pred_vals = pred.values().flatten() @@ if retrain_flags[node_label]: print(f"Training/Re-training model for {node_label}") - ts = TimeSeries.from_dataframe( - df_node, "timestamp", "total_power", fill_missing_dates=True, freq="60s" - ) - model = LinearRegressionModel(lags=10) + candidate_model = LinearRegressionModel(lags=10) try: - model.fit(ts) - model.save(model_path) + candidate_model.fit(ts) + candidate_model.save(model_path) + model = candidate_model except Exception as e: print(f"Error training model for {node_label}: {e}") + if model is None: + node_base_forecasts[node_label] = df_node["total_power"].mean() + continue forecast = model.predict(n=HORIZON_LENGTH, series=ts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around lines 211 - 243, The training-failure bug occurs because the code overwrites a previously loaded model with a new unfitted LinearRegressionModel (model = LinearRegressionModel(lags=10)) and then calls model.predict() even if model.fit()/model.save() failed; fix by preserving the loaded model in a separate variable (e.g., loaded_model = LinearRegressionModel.load(model_path)) and only replace it after fit and save succeed, or on fit/save exception fall back to the preserved loaded_model; if no loaded_model exists, skip calling predict on an unfitted model and instead compute a safe fallback (e.g., mean of the recent series ts or set node_base_forecasts[node_label] to np.nan) and log the condition so predict() is never invoked on an unfitted instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dc6b356-0793-4487-b49d-013128246d28
⛔ Files ignored due to path filters (12)
dynamic-scoring-framework/docs/res/architecture.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/concept-overview.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/concept.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/mcp-connection.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/optimization-architecture-overview.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/optimization-details.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/policy-based-configuration.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/policy-compliant-app-deployment.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/policy-watcher.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/scorer-relocation.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/story1.drawio.pngis excluded by!**/*.pngdynamic-scoring-framework/docs/res/story2.drawio.pngis excluded by!**/*.png
📒 Files selected for processing (94)
dynamic-scoring-framework/Dockerfiledynamic-scoring-framework/Dockerfile.addondynamic-scoring-framework/Makefiledynamic-scoring-framework/README.mddynamic-scoring-framework/deploy/skupper/skupper-site-hub.yamldynamic-scoring-framework/docs/concept-and-design.mddynamic-scoring-framework/docs/development.mddynamic-scoring-framework/docs/install-on-ocp.mddynamic-scoring-framework/docs/optimization-using-dsf.mddynamic-scoring-framework/docs/quickstart.mddynamic-scoring-framework/docs/scoring-api-samples.mddynamic-scoring-framework/docs/setup-local-clusters.mddynamic-scoring-framework/docs/setup-prometheus.mddynamic-scoring-framework/docs/setup-skupper.mddynamic-scoring-framework/hack/image-build/buildconfig-addon-aarch64.yamldynamic-scoring-framework/hack/image-build/buildconfig-policy-watcher-aarch64.yamldynamic-scoring-framework/hack/reset_skupper.shdynamic-scoring-framework/samples/ai-workload-perf-scorer.yamldynamic-scoring-framework/samples/ai-workload-power-scorer.yamldynamic-scoring-framework/samples/ai-workload-scorer/Dockerfiledynamic-scoring-framework/samples/ai-workload-scorer/README.mddynamic-scoring-framework/samples/ai-workload-scorer/app/main.pydynamic-scoring-framework/samples/ai-workload-scorer/app/schemas/__init__.pydynamic-scoring-framework/samples/ai-workload-scorer/app/schemas/config.pydynamic-scoring-framework/samples/ai-workload-scorer/app/schemas/scoring.pydynamic-scoring-framework/samples/ai-workload-scorer/app/static/config.jsondynamic-scoring-framework/samples/ai-workload-scorer/hack/test_scoring.shdynamic-scoring-framework/samples/ai-workload-scorer/manifests/ai-workload-scorer.yamldynamic-scoring-framework/samples/ai-workload-scorer/static/data_performance.jsondynamic-scoring-framework/samples/ai-workload-scorer/static/data_power.jsondynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/Dockerfiledynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/README.mddynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/deployment.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/examples/output.jsondynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/examples/output.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/examples/params.jsondynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/examples/params.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/main.pydynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/clustersetbindings.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/mwrs-app01.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/mwrs-app02.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-disable-mig-cluster1.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-disable-mig-cluster2.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-enable-mig-2g-cluster1.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-enable-mig-2g-cluster2.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-enable-mig-3g-cluster1.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/manifests/policy-enable-mig-3g-cluster2.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/opt.pydynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/pyproject.tomldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/templates/placement-app.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/templates/placement-policy.yamldynamic-scoring-framework/samples/dynamic-scoring-framework-mcp/templates/placementbinding.yamldynamic-scoring-framework/samples/llm-forecast-scorer/Dockerfiledynamic-scoring-framework/samples/llm-forecast-scorer/README.mddynamic-scoring-framework/samples/llm-forecast-scorer/app/contexts/tendency.txtdynamic-scoring-framework/samples/llm-forecast-scorer/app/contexts/tendency.txt.orgdynamic-scoring-framework/samples/llm-forecast-scorer/app/llm-test.pydynamic-scoring-framework/samples/llm-forecast-scorer/app/main.pydynamic-scoring-framework/samples/llm-forecast-scorer/app/schemas/__init__.pydynamic-scoring-framework/samples/llm-forecast-scorer/app/schemas/config.pydynamic-scoring-framework/samples/llm-forecast-scorer/app/schemas/scoring.pydynamic-scoring-framework/samples/llm-forecast-scorer/app/templates/request_evaluation.j2dynamic-scoring-framework/samples/llm-forecast-scorer/hack/test_scoring.shdynamic-scoring-framework/samples/llm-forecast-scorer/static/data.jsondynamic-scoring-framework/samples/mydynamicscorer-example-performance.yamldynamic-scoring-framework/samples/mydynamicscorer-example-powerconsumption.yamldynamic-scoring-framework/samples/mydynamicscorer-external-llm.yamldynamic-scoring-framework/samples/mydynamicscorer-simple-prediction.yamldynamic-scoring-framework/samples/mydynamicscoringconfig.yamldynamic-scoring-framework/samples/policy-watcher/Dockerfiledynamic-scoring-framework/samples/policy-watcher/README.mddynamic-scoring-framework/samples/policy-watcher/deployment.yamldynamic-scoring-framework/samples/policy-watcher/main.pydynamic-scoring-framework/samples/policy-watcher/pyproject.tomldynamic-scoring-framework/samples/sample-scorer/README.mddynamic-scoring-framework/samples/simple-prediction-scorer/Dockerfiledynamic-scoring-framework/samples/simple-prediction-scorer/README.mddynamic-scoring-framework/samples/simple-prediction-scorer/app/get_metrics.pydynamic-scoring-framework/samples/simple-prediction-scorer/app/main.pydynamic-scoring-framework/samples/simple-prediction-scorer/app/schemas/__init__.pydynamic-scoring-framework/samples/simple-prediction-scorer/app/schemas/config.pydynamic-scoring-framework/samples/simple-prediction-scorer/app/schemas/scoring.pydynamic-scoring-framework/samples/simple-prediction-scorer/hack/test_scoring.shdynamic-scoring-framework/samples/simple-prediction-scorer/manifests/cpu-load-generator.yamldynamic-scoring-framework/samples/simple-prediction-scorer/manifests/manifestwork.yamldynamic-scoring-framework/samples/simple-prediction-scorer/manifests/simple-prediction-scorer.yamldynamic-scoring-framework/samples/simple-prediction-scorer/static/sample_cpu_load.jsondynamic-scoring-framework/samples/static-scorer/Dockerfiledynamic-scoring-framework/samples/static-scorer/README.mddynamic-scoring-framework/samples/static-scorer/app/main.pydynamic-scoring-framework/samples/static-scorer/app/schemas/__init__.pydynamic-scoring-framework/samples/static-scorer/app/schemas/config.pydynamic-scoring-framework/samples/static-scorer/app/schemas/scoring.pydynamic-scoring-framework/samples/static-scorer/manifests/manifestwork.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dynamic-scoring-framework/Makefile`:
- Line 186: The Docker build command in the Makefile puts the build context (.)
before flags which is invalid; update the recipe that uses $(CONTAINER_TOOL) and
variables IMG_ADDON and IMAGE_BUILD_EXTRA_FLAGS so all options (e.g., -t
${IMG_ADDON}, $(IMAGE_BUILD_EXTRA_FLAGS), -f Dockerfile.addon, --no-cache) come
before the build context and the context (.) is the final argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 841da01e-beb8-4786-8d7c-8f952f30b767
📒 Files selected for processing (1)
dynamic-scoring-framework/Makefile
Signed-off-by: Kazuma Takeuchi <kazuma.takeuchi02@g.softbank.co.jp>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py (3)
267-270: Misleading name:app_device_max_poweris a per-timestamp mean.
df_concat["app_device_max_power"] = df_concat.mean(axis=1)stores the cross-node mean, then.max()on line 270 takes the temporal maximum. The variable name reads as "maximum power across nodes", which is the opposite aggregation. Considerapp_device_mean_powerfor the column and keepingapp_device_maxfor the temporal max, or switching tomax(axis=1)if that was the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around lines 267 - 270, The column name app_device_max_power is misleading because df_concat["app_device_max_power"] = df_concat.mean(axis=1) computes a per-timestamp cross-node mean and then app_device_max = df_concat["app_device_max_power"].max() takes the temporal max; rename the column to app_device_mean_power (or similar) to reflect the cross-node mean and keep app_device_max as the temporal maximum, or if the original intent was per-timestamp max use df_concat.max(axis=1) instead—update references to df_concat, node_histories, app_device_max_power and app_device_max accordingly.
57-61: Typo:dimentions→dimensions.The identifier is misspelled at declaration (line 57) and reused at lines 132 and 251. Worth renaming once now, while there are no external callers, to keep logs and future additions consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around lines 57 - 61, Rename the misspelled variable "dimentions" to "dimensions" throughout the file: update its declaration (the list comprehension that builds (application["name"], device) pairs) and every usage site where "dimentions" is referenced (including the later uses around the scoring/iteration logic and any logging). Ensure you update all occurrences (declaration and all reads/writes) so names are consistent and tests/locals still compile.
121-154: Drop the commented-out debug blocks.Lines 121-123 and 150-154 are dead debug/scaffolding left behind. Prefer removing them (or gating behind a log level) so the request path stays readable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around lines 121 - 154, Remove the dead commented-out debug scaffolding: delete the commented print header loop (the three commented lines above data = payload.data) and the commented conditional checks at the end of the loop (the commented ifs that reference app_name, gpu_device, MIN_LENGTH and the print/continue for value_label); if you want to preserve debugging capability instead gate those messages behind a logger debug call (use the existing logging facility) rather than leaving commented code, and ensure you adjust references around dim_relevant_cols, resolve_device_profile, resolve_application_name, value_label, and the main loop over payload.data accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py`:
- Around line 192-199: Reindexing df_node to full_index only forward-fills
total_power, leaving per-pod value_* columns as NaN which later causes
df_subset.sum(axis=1) to treat gaps as zeros and skew
node_power/app_device_max_power; update the reindex handling in the df_node
construction (around df_node.set_index("timestamp").reindex(full_index) before
assigning total_power) to forward-fill all per-pod value columns (value_*), or
alternatively drop rows that were introduced solely by reindexing before
computing df_subset.sum(axis=1); ensure you reference df_node, full_index,
total_power, the value_* columns and the later df_subset.sum(axis=1)/node_power
calculation when making the fix so the aggregation uses carried-forward pod
values instead of zeros.
- Around line 54-55: The import-time open("static/config.json", "r") call in
app.main is using a relative CWD-dependent path; change it to resolve the file
relative to this module (use __file__ and pathlib.Path to build
Path(__file__).parent / "static" / "config.json") and reopen with
encoding="utf-8" so loading into the config variable works regardless of how the
process is started (e.g., uvicorn app.main:app).
- Around line 211-243: The code can call model.predict on an unfitted model
because you unconditionally replace a loaded model with
LinearRegressionModel(lags=10) before fit() and swallow fit exceptions; to fix,
preserve any previously-loaded model (keep a loaded_model variable from the
os.path.exists branch), only replace/assign model with the newly-trained
instance after model.fit(ts) and model.save(model_path) succeed, and if fit()
raises, log the error and skip forecasting for this node (e.g., continue the
loop or set node_base_forecasts[node_label] to a safe sentinel) instead of
calling model.predict; refer to model_path, LinearRegressionModel, model.fit,
model.save, model.predict, ts, and node_label to locate and update the logic.
---
Nitpick comments:
In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py`:
- Around line 267-270: The column name app_device_max_power is misleading
because df_concat["app_device_max_power"] = df_concat.mean(axis=1) computes a
per-timestamp cross-node mean and then app_device_max =
df_concat["app_device_max_power"].max() takes the temporal max; rename the
column to app_device_mean_power (or similar) to reflect the cross-node mean and
keep app_device_max as the temporal maximum, or if the original intent was
per-timestamp max use df_concat.max(axis=1) instead—update references to
df_concat, node_histories, app_device_max_power and app_device_max accordingly.
- Around line 57-61: Rename the misspelled variable "dimentions" to "dimensions"
throughout the file: update its declaration (the list comprehension that builds
(application["name"], device) pairs) and every usage site where "dimentions" is
referenced (including the later uses around the scoring/iteration logic and any
logging). Ensure you update all occurrences (declaration and all reads/writes)
so names are consistent and tests/locals still compile.
- Around line 121-154: Remove the dead commented-out debug scaffolding: delete
the commented print header loop (the three commented lines above data =
payload.data) and the commented conditional checks at the end of the loop (the
commented ifs that reference app_name, gpu_device, MIN_LENGTH and the
print/continue for value_label); if you want to preserve debugging capability
instead gate those messages behind a logger debug call (use the existing logging
facility) rather than leaving commented code, and ensure you adjust references
around dim_relevant_cols, resolve_device_profile, resolve_application_name,
value_label, and the main loop over payload.data accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afa04055-cbdf-4f5a-a3fb-831cd5d1b6c9
📒 Files selected for processing (2)
dynamic-scoring-framework/Makefiledynamic-scoring-framework/samples/ai-workload-scorer/app/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- dynamic-scoring-framework/Makefile
| with open("static/config.json", "r") as f: | ||
| config = json.load(f) |
There was a problem hiding this comment.
Relative config path couples startup to CWD.
open("static/config.json", "r") only works when the process is launched from app/. Anyone running uvicorn app.main:app from the repo root (as is common) or mounting the code at a different prefix in a container will hit FileNotFoundError at import time. Resolve the path relative to this file instead, and set a encoding="utf-8" for portability.
Suggested fix
-with open("static/config.json", "r") as f:
+CONFIG_PATH = os.path.join(os.path.dirname(__file__), "static", "config.json")
+with open(CONFIG_PATH, "r", encoding="utf-8") as f:
config = json.load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around
lines 54 - 55, The import-time open("static/config.json", "r") call in app.main
is using a relative CWD-dependent path; change it to resolve the file relative
to this module (use __file__ and pathlib.Path to build Path(__file__).parent /
"static" / "config.json") and reopen with encoding="utf-8" so loading into the
config variable works regardless of how the process is started (e.g., uvicorn
app.main:app).
| df_node = ( | ||
| df_node.set_index("timestamp") | ||
| .reindex(full_index) | ||
| .assign(total_power=lambda d: d["total_power"].ffill()) | ||
| .reset_index() | ||
| ) | ||
|
|
||
| dfs_node[node_label] = df_node |
There was a problem hiding this comment.
Only total_power is forward-filled after reindex.
reindex(full_index) introduces NaN rows for any gaps in the original timestamps, but the .assign(...) only ffills total_power. The per-pod value_* columns remain NaN, and those same columns are later summed at line 260 (df_subset.sum(axis=1)) to compute per-dimension node_power. pandas silently treats NaN as 0 under skipna=True, so reindex-introduced gaps become dips to zero in the app-device aggregation, skewing both app_device_max_power and the final normalized score. Consider ffill'ing all value columns (or dropping the reindex-only rows from the per-dimension view).
- df_node = (
- df_node.set_index("timestamp")
- .reindex(full_index)
- .assign(total_power=lambda d: d["total_power"].ffill())
- .reset_index()
- )
+ df_node = (
+ df_node.set_index("timestamp")
+ .reindex(full_index)
+ .ffill()
+ .reset_index()
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around
lines 192 - 199, Reindexing df_node to full_index only forward-fills
total_power, leaving per-pod value_* columns as NaN which later causes
df_subset.sum(axis=1) to treat gaps as zeros and skew
node_power/app_device_max_power; update the reindex handling in the df_node
construction (around df_node.set_index("timestamp").reindex(full_index) before
assigning total_power) to forward-fill all per-pod value columns (value_*), or
alternatively drop rows that were introduced solely by reindexing before
computing df_subset.sum(axis=1); ensure you reference df_node, full_index,
total_power, the value_* columns and the later df_subset.sum(axis=1)/node_power
calculation when making the fix so the aggregation uses carried-forward pod
values instead of zeros.
Signed-off-by: Kazuma Takeuchi <kazuma.takeuchi02@g.softbank.co.jp>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py (1)
57-61: Typo:dimentions→dimensions.The misspelling is replicated at lines 57, 132, 171, 258 (and the commented
dimentionsreference at line 150). Worth a rename pass while the API is still a sample.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py` around lines 57 - 61, Rename the misspelled variable `dimentions` to `dimensions` throughout main.py (replace the list comprehension that builds `dimentions`, all references at lines near 132, 171, 258, and the commented occurrence around 150), updating any usages or references in functions or loops that expect `dimentions` so they now use `dimensions`; ensure variable name consistency and adjust any imports/locals if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py`:
- Around line 57-61: Rename the misspelled variable `dimentions` to `dimensions`
throughout main.py (replace the list comprehension that builds `dimentions`, all
references at lines near 132, 171, 258, and the commented occurrence around
150), updating any usages or references in functions or loops that expect
`dimentions` so they now use `dimensions`; ensure variable name consistency and
adjust any imports/locals if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8b977d2-4a28-4858-8e85-758e15226bbd
📒 Files selected for processing (1)
dynamic-scoring-framework/samples/ai-workload-scorer/app/main.py
qiujian16
left a comment
qiujian16
left a comment
There was a problem hiding this comment.
This looks better, thanks! The main question is still the maintainability. For example, if scoring API schema is changed, how could we know whether each example plugin is not broken. How we can verify that with the scoring agent.
I'd like a general verification process for plugin based on scoring API. It does not need to be in included in this PR, but I think we should decide some followup before this one is merged.
|
@qiujian16 |
It might work, I think it would need a api conformance test suite or tools that can be used to verify plugin, and can be enabled in the ci. |
|
please create a followup issue and link to this PR, and it is good to go. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KA-Takeuchi, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
f5844b9
into
open-cluster-management-io:main
3 participants
Summary
As a demo of the Dynamic Scoring Framework, we have added:
dynamic-scoring-framework/samplesdirdynamic-scoring-framework/docs/scoring-api-samples.mddynamic-scoring-framework/docs/optimization-using-dsf.mddynamic-scoring-framework/docs/quickstart.mdWe would appreciate your review.
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Documentation
Chores