Add Sync Machanism for Scope and VaraibleScope. Fix test_fetch_var #37085
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your contribution! |
zhiqiu
reviewed
Nov 10, 2021
| // ScopeListener is the callback to sync changes in Original Scope. We can make | ||
| // it | ||
| // a membership of VariableScope. Here we use inherent. | ||
| class VariableScope : public ScopeBase, public ScopeListener { |
There was a problem hiding this comment.
It seems no need to inherent.
Comment on lines
500
to
508
| void SetScope(Scope* outer_scope) { | ||
| // We don't consider SetScope twice currently. | ||
| PADDLE_ENFORCE_EQ( | ||
| outer_scope_, nullptr, | ||
| platform::errors::PreconditionNotMet( | ||
| "You have been SetScope(), please don't call SetScope twice.")); | ||
| outer_scope_ = outer_scope; | ||
| outer_scope->AddListener(this); | ||
| } |
There was a problem hiding this comment.
U can directly initialize outer_scope_ in constructor, and do not expose a SetScope method.
paddle/fluid/framework/scope.h
Outdated
| @@ -164,6 +184,7 @@ class Scope : public ScopeBase { | |||
| // Scope in `kids_` are owned by this class. | |||
| mutable std::list<Scope*> kids_; | |||
| const Scope* parent_{nullptr}; | |||
| mutable std::list<ScopeListener*> listeners_; | |||
Aurelius84
reviewed
Nov 10, 2021
| // machanism. | ||
| // ScopeListener is the callback to sync changes in Original Scope. We can make | ||
| // it | ||
| // a membership of VariableScope. Here we use inherent. |
There was a problem hiding this comment.
这里的注释要手动调整下，pre-commit有时候只关注长度是否满足，样式上不是很好看
| const Scope* GetScope() const { return scope_ptr_.get(); } | ||
|
|
||
| ~VariableScope() { | ||
| outer_scope_->DelListener(this); // don't listen anymore. |
There was a problem hiding this comment.
Suggested change
| outer_scope_->DelListener(this); // don't listen anymore. | |
| if(nullptr!=outer_scope_){ | |
| outer_scope_->DelListener(this); // don't listen anymore. | |
| } |
| VLOG(4) << "Add variable: " << name << " through AddVar()"; | ||
| PADDLE_ENFORCE_NE(outer_scope_, nullptr, | ||
| platform::errors::PreconditionNotMet( | ||
| "the outer_scope_ of Variable is nullptr. please " |
There was a problem hiding this comment.
Suggested change
| "the outer_scope_ of Variable is nullptr. please " | |
| "Reqiured the outer_scope_ of Variable is nullptr. please " |
| PADDLE_ENFORCE_NE(outer_scope_, nullptr, | ||
| platform::errors::PreconditionNotMet( | ||
| "the outer_scope_ of Variable is nullptr. please " | ||
| "SetScope() first")); |
There was a problem hiding this comment.
Suggested change
| "SetScope() first")); | |
| "SetScope() firstly")); |
| // map. | ||
| PADDLE_ENFORCE_NE(outer_scope_, nullptr, | ||
| platform::errors::PreconditionNotMet( | ||
| "the outer_scope_ of Variable is nullptr. please " |
There was a problem hiding this comment.
Suggested change
| "the outer_scope_ of Variable is nullptr. please " | |
| "Required the outer_scope_ of Variable is nullptr. please " |
| PADDLE_ENFORCE_NE(outer_scope_, nullptr, | ||
| platform::errors::PreconditionNotMet( | ||
| "the outer_scope_ of Variable is nullptr. please " | ||
| "SetScope() first")); |
There was a problem hiding this comment.
Suggested change
| "SetScope() first")); | |
| "SetScope() firstly")); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR types
Others
PR changes
Others
Describe
Add Sync Machanism for Scope and VaraibleScope. Fix test_fetch_var
背景
放弃了使用继承Scope的方式来实现同步，相反的，将同步机制和Scope的变量存储逻辑进行了分离。使用Listener的模式设置回调函数实现了Scope和VaraibelScope的同步，这样Scope可以作为VariableScope的一个成员，就不需要引入额外的tmpScope和globalScope的同步。
主要改进
TODO
其他的同步逻辑，比如Delete和SubScope。但是目前VariableScope的行为还没完全定义。比如VariableScope目前还不支持Delete。
将ScopeListener作为一个成员。