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

logger manager #45909

Merged
merged 16 commits into from
Sep 20, 2022
Merged

logger manager #45909

merged 16 commits into from
Sep 20, 2022

Conversation

sljlp
Copy link
Contributor

@sljlp sljlp commented Sep 9, 2022

PR types

Others

PR changes

Others

Describe

  • uniform logger manager in FleetAPI.
  • hidde API under distributed/utils which users don't need.

log samples:
image

@paddle-bot
Copy link

paddle-bot bot commented Sep 9, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

Copy link
Contributor

@gongweibao gongweibao left a comment

Choose a reason for hiding this comment

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

logger does not belong to Fleet class.

@@ -1335,17 +1329,13 @@ def _minimize_impl(self,
no_grad_set=no_grad_set)

if meta_optimizer:
# print("before minimize program id:", id(loss.block.program))
Copy link
Contributor

Choose a reason for hiding this comment

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

logger.debug()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

optimize_ops, params_grads = meta_optimizer.minimize(
loss, startup_program, parameter_list, no_grad_set=no_grad_set)
# print("after minimize program id:", id(loss.block.program))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

# limitations under the License.

__all__ = [ #noqa
'get_host_name_ip',
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these codes for the users?

Copy link
Contributor Author

@sljlp sljlp Sep 13, 2022

Choose a reason for hiding this comment

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

removed

Copy link
Contributor

@gongweibao gongweibao left a comment

Choose a reason for hiding this comment

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

Small comments.

@@ -1463,3 +1479,41 @@ def _minimize_losses_impl(self,
fleet.util._set_strategy(context["valid_strategy"])

return optimize_ops, params_grads

def setLogLevel(self, level):
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to log_util

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


__all__ = []

from .log_utils import get_logger
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove them in the future PR.

gongweibao
gongweibao previously approved these changes Sep 14, 2022
Copy link
Contributor

@gongweibao gongweibao left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -90,3 +91,6 @@
shrink = fleet.shrink
get_hybrid_communicate_group = fleet.get_hybrid_communicate_group
distributed_scaler = distributed_scaler
set_log_level = log_util.set_log_level
get_log_level_code = log_util.get_log_level_code
Copy link
Contributor

@gongweibao gongweibao Sep 14, 2022

Choose a reason for hiding this comment

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

get_log_level one is enough!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping

gongweibao
gongweibao previously approved these changes Sep 15, 2022
Copy link
Contributor

@gongweibao gongweibao left a comment

Choose a reason for hiding this comment

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

LGTM

fuyinno4
fuyinno4 previously approved these changes Sep 15, 2022
liuTINA0907
liuTINA0907 previously approved these changes Sep 15, 2022
@sljlp sljlp dismissed stale reviews from liuTINA0907 and fuyinno4 via 9f950fb September 15, 2022 11:52
fuyinno4
fuyinno4 previously approved these changes Sep 18, 2022
sljlp and others added 2 commits September 18, 2022 12:50
* fix apis global scatter/gather

* update
XiaoguangHu01
XiaoguangHu01 previously approved these changes Sep 18, 2022
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

XiaoguangHu01
XiaoguangHu01 previously approved these changes Sep 19, 2022
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

fuyinno4
fuyinno4 previously approved these changes Sep 19, 2022
liuTINA0907
liuTINA0907 previously approved these changes Sep 19, 2022
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jzhang533 jzhang533 left a comment

Choose a reason for hiding this comment

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

  • 如果API是设计为只限内部调用的,建议API的名称有特殊的标识,常见的做法是用_开头命名。
  • API不公开,不代表API不需要写文档。

import logging


def get_logger(log_level, name="root"):
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need another 'get_logger'? there are multiple versions of this tiny utility.
$ grep -irn 'def get_logger' python
python/paddle/distributed/utils/log_utils.py:18:def get_logger(log_level, name="root"):
python/paddle/distributed/auto_parallel/utils.py:31:def get_logger(log_level, name="auto_parallel"):
python/paddle/distributed/fleet/utils/http_server.py:30:def get_logger(name, level, fmt):
python/paddle/distributed/fleet/launch_utils.py:267:def get_logger(log_level=20, name="root"):
python/paddle/fluid/tests/unittests/auto_checkpoint_utils.py:44:def get_logger():
python/paddle/fluid/incubate/fleet/utils/http_server.py:25:def get_logger(name, level, fmt):
python/paddle/fluid/log_helper.py:22:def get_logger(name, level, fmt=None):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be resolved later.

# See the License for the specific language governing permissions and
# limitations under the License.

__all__ = []
Copy link
Contributor

Choose a reason for hiding this comment

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

APIs are only hidden from here. But APIs are still accessible by

from paddle.distributed.utils.launch_utils import get_cluster, get_gpus, get_cluster_from_args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering to add a prefix '_'.

Copy link
Contributor

Choose a reason for hiding this comment

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

下划线的意思是模块内部,也就是一个文件内部。
加不加两可之间。我还没有找到这个方面的标准。
但是不能主动暴露给用户。

@sljlp sljlp merged commit 264ad20 into PaddlePaddle:develop Sep 20, 2022
fuyinno4 pushed a commit that referenced this pull request Sep 22, 2022
uniform logger manager in FleetAPI.
hidde API under distributed/utils which users don't need.
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.

None yet

7 participants