Skip to content
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

feat: chainer profiling #1552

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat: chainer profiling #1552

wants to merge 1 commit into from

Conversation

Ihab-Asaad
Copy link
Contributor

No description provided.

@@ -23,6 +23,7 @@
from deeppavlov.core.common.errors import ConfigError
from deeppavlov.core.common.registry import register
from deeppavlov.core.data.data_learning_iterator import DataLearningIterator
from deeppavlov.core.models import component
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this import

Comment on lines +297 to +299
if self._chainer.hist_name is not None:
self._chainer.print_hist()

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this lines in case the user wants to see the statistics, otherwise how can we use this addition?

"""Build and return the model described in corresponding configuration file."""
config = parse_config(config)

Copy link
Collaborator

Choose a reason for hiding this comment

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

return line

Comment on lines +269 to +270
if self._chainer.hist_name is not None:
self._chainer.print_hist()
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

@@ -265,5 +266,6 @@ def evaluate(self, iterator: DataLearningIterator, evaluation_targets: Optional[
res[data_type] = report
if print_reports:
print(json.dumps({data_type: report}, ensure_ascii=False, cls=NumpyArrayEncoder))

Copy link
Collaborator

Choose a reason for hiding this comment

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

return line

if in_keys:
res = component.__call__(**dict(zip(in_keys, x)))
else:
res = component.__call__(*x)
duration = time.perf_counter() - start_time
if hist is not None:
hist.labels(component = component).observe(duration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

replace whitespaces around = and use component.__class__.__name__ instead of component as value


@staticmethod
def _compute(*args, param_names, pipe, targets):
def _compute(*args, param_names, pipe, targets, hist: Optional[Histogram] = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove @staticmethod from _compute. After that there will be no need to pass hist as argument, it can be used as self.hist


def __call__(self, *args):
return self._compute(*args, param_names=self.in_x, pipe=self.pipe, targets=self.out_params)
return self._compute(*args, param_names=self.in_x, pipe=self.pipe, targets=self.out_params, hist = self.hist)
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert after removing @staticmethod from _compute

@@ -201,13 +222,13 @@ def compute(self, x, y=None, targets=None):
args += list(zip(*y))
in_params += self.in_y

return self._compute(*args, pipe=pipe, param_names=in_params, targets=targets)
return self._compute(*args, pipe=pipe, param_names=in_params, targets=targets, hist = self.hist)
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert after removing @staticmethod from _compute


model = Chainer(model_config['in'], model_config['out'], model_config.get('in_y'))

model = Chainer(model_config['in'], model_config['out'], model_config.get('in_y'), model_config.get('buckets'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

get buckets and histogram name from model_config['metadata']['profiling']
If there is no such field, don't initialize histogram, if there is such dict with parameters, initialize histogram here.
Initialized histogram should be passed to Chainer instead of buckets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so statistics will be valid in inference mode only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initialize hist in case "profiling" exist without "buckets" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants