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

Feature/shuffle reader #8991

Merged
merged 7 commits into from
Mar 14, 2018
Merged

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Mar 12, 2018

No description provided.


namespace paddle {
namespace framework {

class ReaderBase {
public:
explicit ReaderBase(const std::vector<DDim>& shapes) : shapes_(shapes) {
PADDLE_ENFORCE(!shapes_.empty());
}
virtual void ReadNext(std::vector<LoDTensor>* out) = 0;

virtual void ReInit() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry not related for this PR, just for discussion:

Do we need ReInit for ReaderBase? Maybe readers like network reader is a stream of data, can not be reinitialized back to the beginning.

What is the use case for ReInit? If we don't have a clear use case we probably should drop this method from the base class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For a network reader, we can just throw an exception.

The reader module is in flux. I just discussed the design doc with @JiayiFeng yesterday. Whole design doc will be written soon, maybe within 1-2 days.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the prompt reply!

DDim shape(size_t idx) const;
std::vector<DDim> shapes() const { return shapes_; }
void set_shapes(const std::vector<DDim>& shapes) { shapes_ = shapes; }

virtual bool HasNext() const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you think of replacing HasNext with pre-defined error code returned from ReadNext (e.g., EOF indicates does not have next).

enum_range.insert("CPU");
AddAttr<std::string>("place", "The double buffer place, default is CPU")
.SetDefault("CPU")
.InEnum({enum_range});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't 'CPU' be in this enum?

Item() : ctx_(nullptr) {}

std::vector<framework::LoDTensor> payloads_;
platform::DeviceContext* ctx_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that this ctx_ will only hold gpu_ctx. So gpu_ctx_ might be a better name.

framework::Channel<std::vector<framework::LoDTensor>>* buffer_;
framework::Channel<Item>* buffer_;
platform::Place place_;
std::vector<std::unique_ptr<platform::DeviceContext>> ctxs_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The DoubleBufferReader only takes one Place and all ctx would be the same. So why do we need a vector here?

Copy link
Collaborator

@JiayiFeng JiayiFeng left a comment

Choose a reason for hiding this comment

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

Although there are some small flaws in this PR, I prefer to merge it ASAP because it's the foundation of our further developing on C++ reader. Issues(if there is any) can be fixed by following PRs.

@reyoung reyoung merged commit 48f213e into PaddlePaddle:develop Mar 14, 2018
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.

3 participants