(1) Fix /0 bug in double_ensemble, (2) remove _default_uri for R/expm, (3) support model size in pytorch models #314

D-X-Y · 2021-03-08T06:27:57Z

Four changes:

Fix /0 bug in double_ensemble
simplify the logic of default_uri for R/expm
support model size in pytorch models

Description

In double_ensemble models, it might potentially divide zero and raise an error.
Currently, _default_uri for R maybe inconsistent with that in C.
In pytorch-based contrib codes, there are some useless kwargs, such as verbose.
In pytorch-based contrib codes, add a function to count the number of parameters.

Motivation and Context

Make it more friendly for researchers.

How Has This Been Tested?

Pass the test by running: pytest qlib/tests/test_all_pipeline.py under upper directory of qlib.

Screenshots of Test Results (if appropriate):

I run pytest -W ignore::DeprecationWarning qlib/tests/test_all_pipeline.py, below is the screenshot:

Types of changes

Fix bugs
Add new feature

Derek-Wds · 2021-03-08T07:21:15Z

qlib/workflow/expm.py

@@ -33,6 +33,10 @@ def __repr__(self):
            name=self.__class__.__name__, duri=self._default_uri, curi=self._current_uri
        )

+    def reset_default_uri(self, uri: Text):
+        self._default_uri = uri


Is any reason that you want to add this method? From my perspective, default_uri should be initialized and fixed when users call qlib.init. It seems there is no need to have such method here.

A simple case is:
after we run many different experiments (with different uri), we may want to compare their performance. To query the metrics of each of these experiments, we need to let R know their uri.

@D-X-Y We have uri in C, default_uri and current_uri here.
I think this will make users confusing.
Do you think combining uri in C and default_uri is a good idea?

Derek-Wds · 2021-03-08T07:23:50Z

qlib/workflow/recorder.py

+        return self._artifact_uri
+
+    @property
+    def root_uri(self):


What is this method for? Can you give us more explanation about it?

This is to return the path of a specific record. For example, you can call R.get_recorder().root_uri and it will return: ml-runs/2/32792162f0f44951b1285c0b2e489252.

Please update the related docs.

Derek-Wds · 2021-03-08T07:24:57Z

qlib/workflow/__init__.py

@@ -282,6 +285,12 @@ def get_uri(self):
        """
        return self.exp_manager.uri

+    def reset_default_uri(self, uri: Text):


Same as the one in expm.

Yeap, to provide an api to reset default uri. Use to query experiments with different uri.

qlib/contrib/model/pytorch_utils.py

Derek-Wds · 2021-03-08T07:43:48Z

qlib/log.py

@@ -112,6 +108,27 @@ def set_log_with_config(log_config: dict):
    logging_config.dictConfig(log_config)


+def set_log_basic_config(filename: Optional[Text] = None, format: Optional[Text] = None, level: Optional[int] = None):


Most of the loggers used in Qlib are initialized through the function get_module_logger. If you want to do some enhancement about setting the logger, you may want to make some modifications upon that function instead of creating a new one. This function seems to be a little bit redundant.

The motivation is that get_module_logger is called in different separate Python files, and it is hard for users to control the detailed behavior of get_module_logger function in each file.
This function provides a global control for logging, a case is that before each experiment runs, we can use this function to save the following logs into a file under the recorder directory (without caring about the kwargs of get_module_logger in each file).

We think config logging using Config will make it easier for user to manage all the configurations.
https://github.com/microsoft/qlib/blob/main/qlib/config.py#L266

@D-X-Y
You can set it in the C instead of create a new function

We think the original Python logging system is not user-friendly.
Qlib may use loguru in the future #7

you-n-g · 2021-03-08T08:31:02Z

qlib/contrib/model/pytorch_utils.py

+import torch.nn as nn
+
+
+def count_parameters(models_or_parameters, unit="mb"):


What do you mean by mb?
If it is a storage size unit. the byte length of each number should be considered.
Otherwise, please give explanation of the b

If it is a storage size unit, please use 2**10 instead of 1e3

you-n-g · 2021-03-08T08:42:10Z

qlib/log.py

@@ -112,6 +108,27 @@ def set_log_with_config(log_config: dict):
    logging_config.dictConfig(log_config)


+def set_log_basic_config(filename: Optional[Text] = None, format: Optional[Text] = None, level: Optional[int] = None):


We think config logging using Config will make it easier for user to manage all the configurations.
https://github.com/microsoft/qlib/blob/main/qlib/config.py#L266

@D-X-Y
You can set it in the C instead of create a new function

you-n-g · 2021-03-08T08:43:24Z

qlib/log.py

@@ -112,6 +108,27 @@ def set_log_with_config(log_config: dict):
    logging_config.dictConfig(log_config)


+def set_log_basic_config(filename: Optional[Text] = None, format: Optional[Text] = None, level: Optional[int] = None):


We think the original Python logging system is not user-friendly.
Qlib may use loguru in the future #7

you-n-g · 2021-03-08T08:58:48Z

qlib/workflow/expm.py

@@ -33,6 +33,10 @@ def __repr__(self):
            name=self.__class__.__name__, duri=self._default_uri, curi=self._current_uri
        )

+    def reset_default_uri(self, uri: Text):
+        self._default_uri = uri


@D-X-Y We have uri in C, default_uri and current_uri here.
I think this will make users confusing.
Do you think combining uri in C and default_uri is a good idea?

you-n-g · 2021-03-08T09:03:57Z

qlib/workflow/recorder.py

+    def root_uri(self):
+        start_str = "file:"
+        if self.artifact_uri is not None:
+            xpath = self.artifact_uri.strip(start_str)


lstrip will be more accurate

you-n-g · 2021-03-08T09:07:19Z

qlib/workflow/recorder.py

+        return self._artifact_uri
+
+    @property
+    def root_uri(self):


If the funtion aims to get the path in a filesystem, it should raise error(e.g. NotImplementError) when the uri is not file system.

Besides, the root_uri is not a good name for user.

D-X-Y · 2021-03-09T09:14:54Z

@you-n-g do you have any suggested name for root_uri?

… as get_local_dir

D-X-Y · 2021-03-11T03:12:33Z

Following our offline discussion, I have:

temporarily removed set_log functions
simplify the logic of default uri in expm
rename root_uri to get_local_dir with more robust logic and doc
update count_parameters and add doc

Please let me know if you have any other concerns about the current version.

qlib/contrib/backtest/account.py

Derek-Wds

LGTM. Thanks~

D-X-Y · 2021-03-11T06:47:30Z

@Derek-Wds Thanks for your prompt response.

D-X-Y added 11 commits March 5, 2021 07:15

Add set_log_basic_config function support re-directing log stream

19d9374

Merge branch 'fshare' of github.com:D-X-Y/qlib into fshare

4f980a0

Merge branch 'main' of github.com:microsoft/qlib into fshare

b40bfb1

Merge branch 'main' of github.com:D-X-Y/qlib into fshare

b14a559

Add count_parameters for pytorch models in contrib

131f0e2

Show model size for pytorch models

49697b1

Add reset_default_uri func for R and expm

91fd53a

Remove useless verbose kwarg

73b7107

Avoid dividing zero in model.double_ensemble

d13c9ae

Merge branch 'main' of github.com:microsoft/qlib into fshare

4266492

Fix python format by black

7bed3b4

Derek-Wds added bug Something isn't working enhancement New feature or request labels Mar 8, 2021

Derek-Wds reviewed Mar 8, 2021

View reviewed changes

D-X-Y added 3 commits March 8, 2021 08:16

Simplify count_parameters

ca48345

Fix bugs in count_parameters

03ef918

Fix lint error with Black

e061443

you-n-g reviewed Mar 8, 2021

View reviewed changes

D-X-Y added 2 commits March 11, 2021 02:33

Remove set_log_basic_config, refine count_parameters, rename root_uri…

f6ed175

… as get_local_dir

Refine default uri in expm

cda96be

Fix bugs for get_local_dir

0ef7c8e

D-X-Y requested review from you-n-g and Derek-Wds March 11, 2021 03:10

Derek-Wds reviewed Mar 11, 2021

View reviewed changes

qlib/contrib/backtest/account.py Show resolved Hide resolved

D-X-Y requested a review from Derek-Wds March 11, 2021 04:54

Derek-Wds approved these changes Mar 11, 2021

View reviewed changes

you-n-g merged commit e8beaa5 into microsoft:main Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(1) Fix /0 bug in double_ensemble, (2) remove _default_uri for R/expm, (3) support model size in pytorch models #314

(1) Fix /0 bug in double_ensemble, (2) remove _default_uri for R/expm, (3) support model size in pytorch models #314

D-X-Y commented Mar 8, 2021 •

edited

Loading

Derek-Wds Mar 8, 2021

D-X-Y Mar 8, 2021

you-n-g Mar 8, 2021

Derek-Wds Mar 8, 2021

D-X-Y Mar 8, 2021

Derek-Wds Mar 8, 2021

Derek-Wds Mar 8, 2021

D-X-Y Mar 8, 2021

Derek-Wds Mar 8, 2021

D-X-Y Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

you-n-g Mar 8, 2021

D-X-Y commented Mar 9, 2021

D-X-Y commented Mar 11, 2021

Derek-Wds left a comment

D-X-Y commented Mar 11, 2021

		@@ -112,6 +108,27 @@ def set_log_with_config(log_config: dict):
		logging_config.dictConfig(log_config)


		def set_log_basic_config(filename: Optional[Text] = None, format: Optional[Text] = None, level: Optional[int] = None):

		import torch.nn as nn


		def count_parameters(models_or_parameters, unit="mb"):

(1) Fix /0 bug in double_ensemble, (2) remove _default_uri for R/expm, (3) support model size in pytorch models #314

(1) Fix /0 bug in double_ensemble, (2) remove _default_uri for R/expm, (3) support model size in pytorch models #314

Conversation

D-X-Y commented Mar 8, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots of Test Results (if appropriate):

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D-X-Y commented Mar 9, 2021

D-X-Y commented Mar 11, 2021

Derek-Wds left a comment

Choose a reason for hiding this comment

D-X-Y commented Mar 11, 2021

D-X-Y commented Mar 8, 2021 •

edited

Loading