-
Notifications
You must be signed in to change notification settings - Fork 26
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
Recommendation validation post PGM #46
Conversation
Can one of the admins verify this patch? |
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.
Few comments added. Not familiar with code base so will wait for somebody to check this. I will check this again later.
|
||
app.logger.error("----------------------------------") | ||
app.logger.error("The total manifest file for this ecosystem are: %d" % | ||
app.all_package_list_obj.get_all_list_package_length()) |
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.
Is this error?
app.logger.error("----------------------------------") | ||
app.logger.error("The total manifest file for this ecosystem are: %d" % | ||
app.all_package_list_obj.get_all_list_package_length()) | ||
app.logger.error("----------------------------------") |
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.
Please don't do such reports - it will mess our logging services.
@@ -61,25 +72,25 @@ def train_and_save_kronos(): | |||
@app.route('/api/v1/schemas/kronos_scoring', methods=['POST']) | |||
def predict_and_score(): | |||
input_json = request.get_json() | |||
app.logger.error("\n\n\n**********************************************************************") | |||
app.logger.error( | |||
"\n\n\n**********************************************************************") |
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.
The same as above - we will aggregate these reports in production - these logs are circular and such reports do not carry any valuable information. Here and in other places as well.
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.
@fridex so is it changing the error
to info
log or removing the decoration lines?
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.
Please remove "decoration" lines.
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.
@fridex (y)
@staticmethod | ||
def load_package_list(input_manifest_data_store, additional_path, input_ecosystem): | ||
""" | ||
Generate the aggregated manifest list for a given ecosystem. |
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.
Please follow PEP standard for docstrings.
:return: A set() of user_stack + companion_package | ||
""" | ||
|
||
if not None == input_list and not None == companion_package: |
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.
if input_list is not None and companion_package is not None
@fridex Made the requested changes. |
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.
some comments. LGTM otherwise
filename=manifest_filename) | ||
for manifest_content_json in manifest_content_json_list: | ||
manifest_content_dict = dict(manifest_content_json) | ||
ecosystem = manifest_content_dict[MANIFEST_ECOSYSTEM] |
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.
or manifest_content_dict.get(MANIFEST_ECOSYSTEM)
:return: A set() of user_stack + companion_package.""" | ||
|
||
if input_list is not None and companion_package is not None: | ||
input_list = input_list[:] |
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.
infact, you can directly do return set(input_list + companion_package)
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.
@miteshvp we need this because of this issue https://stackoverflow.com/questions/22054698/python-modifying-list-inside-a-function
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.
👍
recommended_dependency_set.remove(alternate_to) | ||
recommended_dependency_set.add(alternate_package) | ||
return recommended_dependency_set | ||
return set() |
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.
or you can directly use return set([alternate_package if x == alternate_to else x for x in input_list])
|
||
:return: The list of companion packages observed among the known dependency set.""" | ||
|
||
input_list = input_list[:] |
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.
don't need this step. you wil have input_list to be used inside this function
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.
@miteshvp we need this because of this issue https://stackoverflow.com/questions/22054698/python-modifying-list-inside-a-function
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
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.
As I've stated above, I'm not that familiar with this on implementation level to get valuable review. If @miteshvp is fine with merging, feel free to hit the merge button. :)
[test] |
Tests are green. |
Check the validation of recommendations after the results are returned by the PGM