-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add changes to fetch Accelerator metrics at the time of recommendation generation #1319
Conversation
Signed-off-by: bharathappali <abharath@redhat.com>
…n generation Signed-off-by: bharathappali <abharath@redhat.com>
4f186c4
to
39f797e
Compare
@@ -315,4 +335,164 @@ public static boolean isInvalidDataSource(DataSourceInfo datasource) { | |||
return datasource == null || datasource.getAuthenticationConfig() == null || | |||
datasource.getAuthenticationConfig().toString().isEmpty(); | |||
} | |||
|
|||
public static void markAcceleratorDeviceStatusToContainer (ContainerData containerData, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reasoning for adding this to autotune/common/utils
? In general, I would try to limit of scope of use for a method to only those that need it and not make too broad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought to use it in services to check if there is GPU attached at the time of experiment creation so I have added it to common utils. Later the design is changed so I thought it's better to leave it in common so that sometime we might use it across the project to check for GPU usage and mark the containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets increase the scope of the method when it is actually needed, I'd prefer to keep it local for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, Can I move it to RecommendationUtils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @dinogun
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This PR is fifth part of the set of PR's which will be raised for adding GPU support #1312
This PR adds changes to fetch Accelerator metrics at the time of recommendation generation
This PR is build on top of #1318, so it needs to be merged after merging #1318
Fixes #1312
Type of change
How has this been tested?
Tests are added in a different PR
Test Configuration
Checklist 🎯
Additional information
None