Update Current Config & variation to have accelerator info#1935
Open
bharathappali wants to merge 11 commits into
Open
Update Current Config & variation to have accelerator info#1935bharathappali wants to merge 11 commits into
bharathappali wants to merge 11 commits into
Conversation
…ion and current Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
…d fail fast Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Contributor
Reviewer's GuideExtends the recommendation engine’s current configuration model to support accelerator (GPU) metadata alongside CPU/memory, introducing polymorphic resource recommendations and utilities to derive accelerator info from metrics and serialize it cleanly in the API. Sequence diagram for deriving current accelerator recommendation from metricssequenceDiagram
participant RecommendationUtils
participant filteredResultsMap
participant IntervalResults
participant MetricResults
participant AcceleratorMetricMetadata
participant MultiResourceRecommendation
participant AcceleratorRecommendationItem
RecommendationUtils->>filteredResultsMap: get(timestampToExtract)
alt hasAcceleratorData
RecommendationUtils->>RecommendationUtils: hasAcceleratorData(IntervalResults)
else findLatestAcceleratorTimestamp
RecommendationUtils->>RecommendationUtils: findLatestAcceleratorTimestamp(filteredResultsMap, timestampToExtract)
RecommendationUtils->>filteredResultsMap: get(realTimestampToExtract)
end
RecommendationUtils->>IntervalResults: getMetricResultsMap()
loop for metric in ACCELERATOR_METRICS
RecommendationUtils->>IntervalResults: getMetricResultsMap().get(metric)
IntervalResults-->>RecommendationUtils: MetricResults
alt metricResult not null
RecommendationUtils->>MetricResults: getMetadata()
MetricResults-->>RecommendationUtils: AcceleratorMetricMetadata
RecommendationUtils->>RecommendationUtils: getAcceleratorRecommendationItem(AcceleratorMetricMetadata)
RecommendationUtils-->>AcceleratorRecommendationItem: AcceleratorRecommendationItem
end
end
RecommendationUtils->>MultiResourceRecommendation: new MultiResourceRecommendation()
RecommendationUtils->>MultiResourceRecommendation: addAcceleratorRecommendationItem(AcceleratorRecommendationItem)
RecommendationUtils-->>MultiResourceRecommendation: return MultiResourceRecommendation
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new
extractCurrentContainerConfigmethod appears incomplete:currentAcceleratorRequest/currentAcceleratorLimitare unused and theACCELERATORSbranch in the limits map is empty, so either wire these through toCurrentContainerConfigValuesor remove the dead code. - The change to use
ResourceRecommendationin thecurrentConfigMapintroduces multiple unchecked casts back toRecommendationConfigItem; consider a more type-safe approach (e.g., generics or separate accessors for CPU/memory vs multi-resource types) to avoid potentialClassCastExceptions and make the API clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `extractCurrentContainerConfig` method appears incomplete: `currentAcceleratorRequest`/`currentAcceleratorLimit` are unused and the `ACCELERATORS` branch in the limits map is empty, so either wire these through to `CurrentContainerConfigValues` or remove the dead code.
- The change to use `ResourceRecommendation` in the `currentConfigMap` introduces multiple unchecked casts back to `RecommendationConfigItem`; consider a more type-safe approach (e.g., generics or separate accessors for CPU/memory vs multi-resource types) to avoid potential `ClassCastException`s and make the API clearer.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/recommendations/engine/BaseRecommendationProcessor.java" line_range="250-251" />
<code_context>
+ null != limitsMap.get(AnalyzerConstants.RecommendationItem.MEMORY)) {
+ currentMemLimit = (RecommendationConfigItem) limitsMap.get(AnalyzerConstants.RecommendationItem.MEMORY);
+ }
+ if (limitsMap.containsKey(AnalyzerConstants.RecommendationItem.ACCELERATORS) &&
+ null != limitsMap.get(AnalyzerConstants.RecommendationItem.ACCELERATORS)) {
+
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Accelerator limits branch is empty and the collected accelerator variables are unused, leaving this method incomplete.
The method declares `currentAcceleratorRequest`/`currentAcceleratorLimit` and checks `RecommendationItem.ACCELERATORS` in `limitsMap`, but never reads the value or sets the accelerator fields on `CurrentContainerConfigValues`. As a result, callers can’t obtain the current accelerator config. Please wire this through by extracting the accelerator recommendation(s) from `limitsMap` and setting the corresponding fields on `CurrentContainerConfigValues`.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/analyzer/recommendations/engine/BaseRecommendationProcessor.java" line_range="314-316" />
<code_context>
+ return acceleratorRequestRecommendationItems;
+ }
+
+ public void setAcceleratorRequestRecommendationItems(List<AcceleratorRecommendationItem> acceleratorRequestRecommendationItems) {
+ if (null != acceleratorRequestRecommendationItems && !acceleratorRequestRecommendationItems.isEmpty())
+ this.acceleratorRequestRecommendationItems = acceleratorRequestRecommendationItems;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Setters silently ignore null/empty lists, which may surprise callers expecting to clear the accelerator recommendations.
Because these setters only assign when the argument is non-null and non-empty, callers cannot clear existing recommendations and may unintentionally leave stale data when passing an empty list. Either allow empty lists (remove the `isEmpty` check) so callers can clear the state, or make these fields immutable and only set via constructor to make the behavior explicit.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/autotune/analyzer/recommendations/MultiResourceRecommendation.java" line_range="35-36" />
<code_context>
+ this.acceleratorRecommendationItems = new ArrayList<>();
+ }
+
+ public MultiResourceRecommendation(List<AcceleratorRecommendationItem> acceleratorRecommendationItems) {
+ this.acceleratorRecommendationItems = acceleratorRecommendationItems;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Constructor allows a null list which can cause NPEs in addAcceleratorRecommendationItem.
The second constructor assigns the parameter directly, so `new MultiResourceRecommendation(null)` followed by `addAcceleratorRecommendationItem` will NPE. Normalize `null` to an empty list in the constructor or lazily initialize the list inside `addAcceleratorRecommendationItem`.
</issue_to_address>
### Comment 4
<location path="src/main/java/com/autotune/analyzer/recommendations/utils/RecommendationUtils.java" line_range="761-766" />
<code_context>
+ return false;
+ }
+
+ for (AnalyzerConstants.MetricName metric : ACCELERATOR_METRICS) {
+ MetricResults metricResult =
+ intervalResults.getMetricResultsMap().get(metric);
+
+ if (metricResult == null || metricResult.getMetadata() == null) {
+ continue;
+ }
+
+ if (AnalyzerConstants.DeviceType.ACCELERATOR.toString()
+ .equalsIgnoreCase(metricResult.getMetadata().getType())) {
+ AcceleratorMetricMetadata acceleratorMetricMetadata = (AcceleratorMetricMetadata) metricResult.getMetadata();
+ if (null != acceleratorMetricMetadata.getModelName())
+ return true;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Casting metadata to AcceleratorMetricMetadata without an instanceof check assumes a strong coupling that may not always hold.
In `hasAcceleratorData`, `metadata.getType()` is checked against `DeviceType.ACCELERATOR`, then `metadata` is blindly cast to `AcceleratorMetricMetadata`. If another `MetricMetadata` implementation returns the same type, this will cause a `ClassCastException`. Either guard the cast with `instanceof AcceleratorMetricMetadata` or enforce the concrete type at the wiring/creation point so this assumption is guaranteed.
```suggestion
if (AnalyzerConstants.DeviceType.ACCELERATOR.toString()
.equalsIgnoreCase(metricResult.getMetadata().getType())
&& metricResult.getMetadata() instanceof AcceleratorMetricMetadata) {
AcceleratorMetricMetadata acceleratorMetricMetadata =
(AcceleratorMetricMetadata) metricResult.getMetadata();
if (acceleratorMetricMetadata.getModelName() != null) {
return true;
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+250
to
+251
| if (limitsMap.containsKey(AnalyzerConstants.RecommendationItem.ACCELERATORS) && | ||
| null != limitsMap.get(AnalyzerConstants.RecommendationItem.ACCELERATORS)) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Accelerator limits branch is empty and the collected accelerator variables are unused, leaving this method incomplete.
The method declares currentAcceleratorRequest/currentAcceleratorLimit and checks RecommendationItem.ACCELERATORS in limitsMap, but never reads the value or sets the accelerator fields on CurrentContainerConfigValues. As a result, callers can’t obtain the current accelerator config. Please wire this through by extracting the accelerator recommendation(s) from limitsMap and setting the corresponding fields on CurrentContainerConfigValues.
Comment on lines
+314
to
+316
| public void setAcceleratorRequestRecommendationItems(List<AcceleratorRecommendationItem> acceleratorRequestRecommendationItems) { | ||
| if (null != acceleratorRequestRecommendationItems && !acceleratorRequestRecommendationItems.isEmpty()) | ||
| this.acceleratorRequestRecommendationItems = acceleratorRequestRecommendationItems; |
Contributor
There was a problem hiding this comment.
suggestion: Setters silently ignore null/empty lists, which may surprise callers expecting to clear the accelerator recommendations.
Because these setters only assign when the argument is non-null and non-empty, callers cannot clear existing recommendations and may unintentionally leave stale data when passing an empty list. Either allow empty lists (remove the isEmpty check) so callers can clear the state, or make these fields immutable and only set via constructor to make the behavior explicit.
Comment on lines
+35
to
+36
| public MultiResourceRecommendation(List<AcceleratorRecommendationItem> acceleratorRecommendationItems) { | ||
| this.acceleratorRecommendationItems = acceleratorRecommendationItems; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Constructor allows a null list which can cause NPEs in addAcceleratorRecommendationItem.
The second constructor assigns the parameter directly, so new MultiResourceRecommendation(null) followed by addAcceleratorRecommendationItem will NPE. Normalize null to an empty list in the constructor or lazily initialize the list inside addAcceleratorRecommendationItem.
Comment on lines
+761
to
+766
| if (AnalyzerConstants.DeviceType.ACCELERATOR.toString() | ||
| .equalsIgnoreCase(metricResult.getMetadata().getType())) { | ||
| AcceleratorMetricMetadata acceleratorMetricMetadata = (AcceleratorMetricMetadata) metricResult.getMetadata(); | ||
| if (null != acceleratorMetricMetadata.getModelName()) | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Casting metadata to AcceleratorMetricMetadata without an instanceof check assumes a strong coupling that may not always hold.
In hasAcceleratorData, metadata.getType() is checked against DeviceType.ACCELERATOR, then metadata is blindly cast to AcceleratorMetricMetadata. If another MetricMetadata implementation returns the same type, this will cause a ClassCastException. Either guard the cast with instanceof AcceleratorMetricMetadata or enforce the concrete type at the wiring/creation point so this assumption is guaranteed.
Suggested change
| if (AnalyzerConstants.DeviceType.ACCELERATOR.toString() | |
| .equalsIgnoreCase(metricResult.getMetadata().getType())) { | |
| AcceleratorMetricMetadata acceleratorMetricMetadata = (AcceleratorMetricMetadata) metricResult.getMetadata(); | |
| if (null != acceleratorMetricMetadata.getModelName()) | |
| return true; | |
| } | |
| if (AnalyzerConstants.DeviceType.ACCELERATOR.toString() | |
| .equalsIgnoreCase(metricResult.getMetadata().getType()) | |
| && metricResult.getMetadata() instanceof AcceleratorMetricMetadata) { | |
| AcceleratorMetricMetadata acceleratorMetricMetadata = | |
| (AcceleratorMetricMetadata) metricResult.getMetadata(); | |
| if (acceleratorMetricMetadata.getModelName() != null) { | |
| return true; | |
| } | |
| } |
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
1 participant
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.
Description
Continuation to #1921
PR #1921 needs to be merged before this PR
This PR adds the updates to the current config, adds new mechanism for adding accelerator info in current and variation
Fixes #1920
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Add support for representing accelerator resource information alongside CPU and memory in current and recommended configurations.
New Features:
Enhancements: