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

[JitLayer]Pybind JitLayer VarBase Function and add python UT #44010

Merged
merged 9 commits into from
Jul 8, 2022

Conversation

0x45f
Copy link
Contributor

@0x45f 0x45f commented Jul 1, 2022

PR types

Others

PR changes

Others

Describe

[JitLayer]Pybind JitLayer VarBase Function and add python UT

python/paddle/fluid/dygraph/jit.py Outdated Show resolved Hide resolved
python/paddle/fluid/dygraph/jit.py Outdated Show resolved Hide resolved
python/paddle/fluid/dygraph/jit.py Outdated Show resolved Hide resolved
paddle/fluid/pybind/pybind.cc Outdated Show resolved Hide resolved
paddle/fluid/pybind/pybind.cc Outdated Show resolved Hide resolved
}

const FunctionMap &CompilationUnit::FunctionDict() const {
return function_dict_;
Copy link
Contributor

Choose a reason for hiding this comment

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

function_dict_ 不是一个合适的命名,因为dict是python的概念。C++里是map,下个PR可以考虑优化下

@@ -32,8 +34,12 @@ class CompilationUnit {
void SetFunction(const std::string &name,
const std::shared_ptr<BaseFunction> &function);

std::vector<std::string> FunctionNames() const;

const FunctionMap &FunctionDict() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

同上,这里的FunctionDict也不是一个好的API名

#include "paddle/phi/core/compat/convert_utils.h"
#include "paddle/phi/core/lod_utils.h"
#include "paddle/utils/none.h"
#ifdef PADDLE_WITH_ASCEND
#include "paddle/fluid/pybind/ascend_wrapper_py.h"
#endif
#include "paddle/fluid/jit/executor_function.h"
#include "paddle/fluid/jit/layer.h"
#include "paddle/fluid/jit/serializer.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

这里三个头文件都是不需要的吧?

from paddle.fluid.core import Load


class Layer():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class Layer():
class Layer:
或者
class Layer(object)

def __init__(self):
self.cpp_layer = None
# {name: Function}
self.funciton_map = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.funciton_map = {}
self.functions = {}

有单词拼写错误,另外这里的dict需要使用WeakRef么?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里应该不需要WeakRef吧,funciton_map 持有的python端的 Function: self.funciton_map[name] = Function(function)。其余的意见已经修改,感谢


for name, function in function_dict.items():
self.funciton_map[name] = Function(function)
setattr(self, name, self.funciton_map[name].__call__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setattr(self, name, self.funciton_map[name].__call__)
setattr(self, name, self.funciton_map[name])

是不是不需要设置__call__,直接setattr对应的Function即可?

Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

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

LGTM

@0x45f 0x45f merged commit 2fc93f3 into PaddlePaddle:develop Jul 8, 2022
@0x45f 0x45f deleted the pybind-jit-layer branch July 8, 2022 09:14
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