Update the existing structures to support accelerator array in variation and current sections#1921
Open
bharathappali wants to merge 1 commit into
Open
Update the existing structures to support accelerator array in variation and current sections#1921bharathappali wants to merge 1 commit into
bharathappali wants to merge 1 commit into
Conversation
…ion and current Signed-off-by: bharathappali <abharath@redhat.com>
Contributor
Reviewer's GuideRefactors recommendation resource typing so CPU/memory maps and accelerator arrays share a common ResourceRecommendation interface, introduces accelerator-specific recommendation structures, and adds a custom Gson adapter to serialize a wrapped list of accelerators directly as a JSON array in current and variation sections. Sequence diagram for MultiResourceRecommendation JSON serialization/deserializationsequenceDiagram
participant Engine as RecommendationEngine
participant gson as Gson
participant multi as MultiResourceRecommendation
participant adapter as MultiResourceRecommendationAdapter
Engine->>gson: toJson(multi)
gson->>adapter: serialize(multi, typeOfSrc, context)
adapter->>adapter: src.getItems()
adapter->>gson: context.serialize(items)
gson-->>Engine: JSON accelerators_array
Engine->>gson: fromJson(json, MultiResourceRecommendation)
gson->>adapter: deserialize(jsonElement, typeOfT, context)
adapter->>gson: context.deserialize(jsonElement, listType)
gson-->>adapter: List AcceleratorRecommendationItem
adapter-->>gson: new MultiResourceRecommendation(items)
gson-->>Engine: MultiResourceRecommendation instance
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 1 issue, and left some high level feedback:
- Switching
Config.requests/limitstoMap<..., ResourceRecommendation>but then downcasting toRecommendationConfigIteminRecommendationEngineweakens type safety and defeats the polymorphic design; consider either separating the maps by resource type or introducing typed accessors/visitors so the engine doesn’t need explicit casts. - The
MultiResourceRecommendationAdapter.deserializemethod assumes the JSON is always an array; you may want to defensively handlenullor non-array inputs (e.g., a single object or empty value) to avoid runtimeJsonParseExceptions on slightly malformed or older payloads. - If
AcceleratorRecommendationItemis intended to be deserialized by Gson in more places, consider adding a no-arg constructor and/or setters so that it plays more naturally with Gson’s default instantiation and to keep it consistent with the other recommendation model classes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Switching `Config.requests`/`limits` to `Map<..., ResourceRecommendation>` but then downcasting to `RecommendationConfigItem` in `RecommendationEngine` weakens type safety and defeats the polymorphic design; consider either separating the maps by resource type or introducing typed accessors/visitors so the engine doesn’t need explicit casts.
- The `MultiResourceRecommendationAdapter.deserialize` method assumes the JSON is always an array; you may want to defensively handle `null` or non-array inputs (e.g., a single object or empty value) to avoid runtime `JsonParseException`s on slightly malformed or older payloads.
- If `AcceleratorRecommendationItem` is intended to be deserialized by Gson in more places, consider adding a no-arg constructor and/or setters so that it plays more naturally with Gson’s default instantiation and to keep it consistent with the other recommendation model classes.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/adapters/MultiResourceRecommendationAdapter.java" line_range="21" />
<code_context>
+ }
+
+ @Override
+ public MultiResourceRecommendation deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
+ throws JsonParseException {
+ Type listType = new com.google.gson.reflect.TypeToken<List<AcceleratorRecommendationItem>>(){}.getType();
</code_context>
<issue_to_address>
**suggestion:** Deserialization does not guard against non-array or null JSON and may produce a MultiResourceRecommendation with a null items list.
The adapter currently assumes `json` can always be deserialized into `List<AcceleratorRecommendationItem>` and passes that (possibly null) list to the constructor. Consider:
- Returning an empty `MultiResourceRecommendation` when `json` is `null` or `JsonNull`.
- Validating that `json` is a `JsonArray` and throwing a clear `JsonParseException` otherwise.
This will keep `items` non-null and make failures more explicit and predictable.
Suggested implementation:
```java
@Override
public MultiResourceRecommendation deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
throws JsonParseException {
// Treat null or JsonNull as an empty recommendation rather than returning a null items list
if (json == null || json.isJsonNull()) {
return new MultiResourceRecommendation(Collections.emptyList());
}
// Ensure the JSON structure is an array; fail fast with a clear error otherwise
if (!json.isJsonArray()) {
throw new JsonParseException(
"Expected a JSON array for MultiResourceRecommendation, but found: " + json.toString());
}
Type listType = new com.google.gson.reflect.TypeToken<List<AcceleratorRecommendationItem>>() {}.getType();
List<AcceleratorRecommendationItem> items = context.deserialize(json, listType);
// Guarantee items is non-null
if (items == null) {
items = Collections.emptyList();
}
return new MultiResourceRecommendation(items);
}
```
To compile successfully, ensure the following import is present at the top of the file (if not already there):
- `import java.util.Collections;`
If it is missing, add it alongside the other imports in this class.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| @Override | ||
| public MultiResourceRecommendation deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) |
Contributor
There was a problem hiding this comment.
suggestion: Deserialization does not guard against non-array or null JSON and may produce a MultiResourceRecommendation with a null items list.
The adapter currently assumes json can always be deserialized into List<AcceleratorRecommendationItem> and passes that (possibly null) list to the constructor. Consider:
- Returning an empty
MultiResourceRecommendationwhenjsonisnullorJsonNull. - Validating that
jsonis aJsonArrayand throwing a clearJsonParseExceptionotherwise.
This will keepitemsnon-null and make failures more explicit and predictable.
Suggested implementation:
@Override
public MultiResourceRecommendation deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
throws JsonParseException {
// Treat null or JsonNull as an empty recommendation rather than returning a null items list
if (json == null || json.isJsonNull()) {
return new MultiResourceRecommendation(Collections.emptyList());
}
// Ensure the JSON structure is an array; fail fast with a clear error otherwise
if (!json.isJsonArray()) {
throw new JsonParseException(
"Expected a JSON array for MultiResourceRecommendation, but found: " + json.toString());
}
Type listType = new com.google.gson.reflect.TypeToken<List<AcceleratorRecommendationItem>>() {}.getType();
List<AcceleratorRecommendationItem> items = context.deserialize(json, listType);
// Guarantee items is non-null
if (items == null) {
items = Collections.emptyList();
}
return new MultiResourceRecommendation(items);
}To compile successfully, ensure the following import is present at the top of the file (if not already there):
import java.util.Collections;
If it is missing, add it alongside the other imports in this class.
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
This PR adds the structural changes needed to export the accelerator info in current and variation sections of the recommendation JSON.
Finalized JSON designs:
Current Section:
{ "current": { "requests": { "memory": { "amount": 490.93, "format": "MiB" }, "cpu": { "amount": 1.46, "format": "cores" } }, "limits": { "memory": { "amount": 712.21, "format": "MiB" }, "cpu": { "amount": 1.54, "format": "cores" }, "accelerators": [ { "model": "nvidia-a100-80gb", "partition": "full-gpu", "compute": { "amount": 7, "format": "slices" }, "memory": { "amount": 80, "format": "GiB" } } ] } } }Variation Section:
{ "variation": { "requests": { "memory": { "amount": 707.05, "format": "MiB" }, "cpu": { "amount": 6.22, "format": "cores" } }, "limits": { "memory": { "amount": 485.77, "format": "MiB" }, "cpu": { "amount": 6.14, "format": "cores" }, "accelerators": [ { "model": "nvidia-a100-80gb", "compute": { "amount": -57.14, "format": "percentage" }, "memory": { "amount": -50.0, "format": "percentage" } } ] } } }From a code stand point you can see that the same structure should support a map for
cpu,memoryand an array for theaccelerator.Maintaining the map with generic
Objectwill cause type check issues and also the de-serialization efforts will be huge, So we went ahead to have a interface which is implemented by two types, also this helps to keep the structure extendable and also we can maintain common functions at interface level in future.As we need to have a wrapper class around the list, direct conversion makes it a nested structure of
itemsas key in the JSON, so we need to have an adapter which skips that structure and adds the list values directly to the parentFixes #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
Introduce a polymorphic resource recommendation model to support both scalar resources and accelerator arrays in recommendation outputs.
New Features:
Enhancements: