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

nn.Graph python #5309

Merged
merged 41 commits into from Jul 7, 2021
Merged

nn.Graph python #5309

merged 41 commits into from Jul 7, 2021

Conversation

strint
Copy link
Contributor

@strint strint commented Jun 24, 2021

nn.Graph

  • _add_block, attr get and set,
  • recursive build Block
  • Block.forward
  • GraphConfig
  • repr
  • _named_state and _state_tensortuple
  • c++ NNGraph 导出

注册Module产生的graph的结构:

g = CustomGraph()
print(repr(g))
>>>
(CustomGraph_15:CustomGraph:GRAPH): (
  (m:CustomModule:MODULE): (
    (layer:SubModule:MODULE): (
      (conv1:Conv2d:MODULE): (
        (weight:Parameter:PARAMETER): ()
        (bias:Parameter:PARAMETER): ()
      )
      (relu:ReLU:MODULE): ()
    )
    (fc1:Linear:MODULE): (
      (weight:Parameter:PARAMETER): ()
      (bias:Parameter:PARAMETER): ()
    )
    (dummy_buff:Tensor:BUFFER): ()
  )
)

@chengtbf
Copy link
Contributor

oneflow/core/job/graph.h 这个文件名会不会 跟 oneflow/core/graph/graph.h 文件名造成混淆?

@strint
Copy link
Contributor Author

strint commented Jun 28, 2021

oneflow/core/job/graph.h 这个文件名会不会 跟 oneflow/core/graph/graph.h 文件名造成混淆?

的确会,叫graph_api.h/.cpp怎么样

@jackalcooper
Copy link
Collaborator

oneflow/core/job/graph.h 这个文件名会不会 跟 oneflow/core/graph/graph.h 文件名造成混淆?

的确会,叫graph_api.h/.cpp怎么样

这个放到 oneflow/api/python 下吧,是 python API 独有的内容不应放到 core 里面?

@chengtbf
Copy link
Contributor

oneflow/core/job/graph.h 这个文件名会不会 跟 oneflow/core/graph/graph.h 文件名造成混淆?
的确会,叫graph_api.h/.cpp怎么样

这个放到 oneflow/api/python 下吧,是 python API 独有的内容不应放到 core 里面?

这个不仅仅是 api,C++ 的 nn.Graph 对象是要有一系列逻辑(类似于之前的 JobBuildAndInferCtx),不是 python api 独有的内容。是否可以放到 framework 路径下? oneflow/core/framework/nn_graph.h ? 还是 oneflow/core/job/nn_graph.h

oneflow/python/nn/graph.py Outdated Show resolved Hide resolved
@chengtbf

This comment has been minimized.

oneflow/python/nn/graph.py Outdated Show resolved Hide resolved

@property
def state_tensortuple(self):
return tensor_tuple_util.convert_to_tensor_tuple(tuple(t for _, t in self._named_state()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

给nn.Graph增加了这个属性,但是感觉state_tensortuple只有内部使用,这个转换是不是适合在使用的地方做

@chengtbf
Copy link
Contributor

chengtbf commented Jul 6, 2021

这个 PR 先不处理 call 相关的复杂逻辑,仅提供一个 nn.Graph 和 NNGraph 的架子。先review 合并了吧 ~

我 和 文骁 @leaves-zwx 都 review 一下,尽早合并。

@chengtbf
Copy link
Contributor

chengtbf commented Jul 6, 2021

先解决一下冲突,转成 正式 PR,并通过 CI @strint

@strint strint marked this pull request as ready for review July 6, 2021 10:10
@strint strint requested review from oneflow-ci-bot and removed request for oneflow-ci-bot July 6, 2021 14:38
@oneflow-ci-bot oneflow-ci-bot removed their request for review July 6, 2021 15:40
@oneflow-ci-bot oneflow-ci-bot removed their request for review July 7, 2021 14:14
@oneflow-ci-bot oneflow-ci-bot merged commit 0a82143 into master Jul 7, 2021
@oneflow-ci-bot oneflow-ci-bot deleted the dev_graph branch July 7, 2021 14:16
@strint strint added this to the v0.5.0 milestone Jul 10, 2021
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.

None yet

7 participants