Skip to content
This repository has been archived by the owner on Jun 7, 2023. It is now read-only.

Refactoring code base to utilize better encapsulation #347

Merged
1 commit merged into from
Oct 31, 2019

Conversation

zak-hassan
Copy link
Contributor

Description

Breaking up code and refactoring. Also include better docs.

@zak-hassan zak-hassan self-assigned this Oct 31, 2019
@sesheta sesheta added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 31, 2019
@ghost
Copy link

ghost commented Oct 31, 2019

Build failed.

@zak-hassan
Copy link
Contributor Author

@4n4nd @HumairAK Please review

@ghost
Copy link

ghost commented Oct 31, 2019

Build succeeded.

@HumairAK
Copy link
Collaborator

lgtm but this should require a 2nd approval, - @4n4nd thoughts?

Copy link

@4n4nd 4n4nd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested minor changes

anomaly_detector/core/pipeline.py Outdated Show resolved Hide resolved
anomaly_detector/core/job.py Outdated Show resolved Hide resolved
anomaly_detector/core/pipeline.py Outdated Show resolved Hide resolved
self.recreate_model = recreate_model

def execute_with_tracing(self, tracer):
"""Will wrap execution of inference with tracer to measure latency."""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execution of training maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It executes the training but wraps a tracer around it. This is needed to make tracing possible to measure how long for example Training takes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure, but the doc string says execution of inference and you're telling me it is the execution of training.

recreate_models = False

def __init__(self, node_map=24, model_adapter=None, recreate_model=True):
"""Initialize inference job with fields to perform model training."""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same training job here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the comments.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring says inference, you say training

@ghost
Copy link

ghost commented Oct 31, 2019

Build succeeded.

@ghost
Copy link

ghost commented Oct 31, 2019

Build succeeded.

@ghost
Copy link

ghost commented Oct 31, 2019

Build failed.

Copy link

@4n4nd 4n4nd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2019
@ghost
Copy link

ghost commented Oct 31, 2019

Build succeeded.

@ghost
Copy link

ghost commented Oct 31, 2019

Build succeeded (gate pipeline).

@ghost ghost merged commit db5abed into AICoE:master Oct 31, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants