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

Fix issues in point of MultiClientSession #5469

Merged
merged 9 commits into from
Jul 13, 2021
Merged

Conversation

leaves-zwx
Copy link
Contributor

No description provided.

oneflow/init.py Outdated Show resolved Hide resolved
if oneflow._oneflow_internal.CurrentMachineId() == 0:
scope_util.InitScopeStack()
else:
exit(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

如果在 Single-Client 下,worker 的 python 进程没有在 env.init 里 exit(0) 的话,会在 cluster 训练完退出以后,尝试执行 env.init 后面的 python 代码然后挂掉吧。这里肯定不能这样删除。

我觉得保险起见,要不提供两个接口? flow.env.init 就强制是 Single-Client 的启动方式。如果在 is_multi_client 下调用 flow.env.init ,就直接弹出提示用户这个接口在 Multi-Client 下没有效果。 然后 Multi-Client 下的 env 初始化(用户不可控)就另外启用一个?

这里的逻辑需要跟 @clackhan @daquexian 讨论一下再确定。或者这个 PR 先不动这里的逻辑。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我觉得可以按你提的建议先修改了,flow.env.init 里先保持在 single-client 下初始化 scope stack,然后 multi-client 下 scope stack 的初始化直接放在 MultiClientSession 被创建后。

@@ -40,6 +41,19 @@ def test(self):
sess.TryInit()
self.assertEqual(sess.status, sess.Status.INITED)

# sess.TryClose()
# self.assertEqual(sess.status, sess.Status.CLOSED)
Copy link
Contributor

Choose a reason for hiding this comment

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

如果支持MultiClientSession的clear,这里是不是就可以写上了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

光 MultiClientSession 支持 clear 不行,还需要在 test_case2 里面重新创建一个 MultiClientSession,而不能用 GetDefaultSession。

不过之前讨论的结果不是 MultiClientSession 永不关闭吗?所有的测试都共用一个 session。

@strint strint mentioned this pull request Jul 13, 2021
6 tasks
@oneflow-ci-bot oneflow-ci-bot requested review from oneflow-ci-bot and removed request for oneflow-ci-bot July 13, 2021 08:50
@strint strint mentioned this pull request Jul 13, 2021
5 tasks
@oneflow-ci-bot oneflow-ci-bot removed their request for review July 13, 2021 10:40
@oneflow-ci-bot oneflow-ci-bot merged commit 4895d9b into master Jul 13, 2021
@oneflow-ci-bot oneflow-ci-bot deleted the fix_multi_client_sess branch July 13, 2021 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants