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

Refine data reader and move data_reader to async_data_reader. #726

Merged
merged 3 commits into from
Mar 13, 2018

Conversation

pkuyym
Copy link
Contributor

@pkuyym pkuyym commented Mar 13, 2018

Resolves #705

@pkuyym pkuyym requested review from kuke and zhxfl March 13, 2018 06:03
Copy link
Collaborator

@kuke kuke left a comment

Choose a reason for hiding this comment

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

Almost LGTM

class CriticalException(Exception):
pass


class SharedNDArray(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some comments for this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

self._is_verify = state[4]


class SharedMemoryPoolManager(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also please add some comments for this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

label_t.set(labels.ndarray, place)
label_t.set_lod([lod.ndarray])

test_data_reader.recycle(features, labels, lod)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we hide the recycle, e.g. move it into the iterator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have considered to hide this function, seems not trivial. We can validate the correctness first.

Copy link
Member

@zhxfl zhxfl left a comment

Choose a reason for hiding this comment

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

AG

Copy link
Member

@zhxfl zhxfl left a comment

Choose a reason for hiding this comment

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

Almost LGTM

self._buf = None
self._array = np.zeros(1, dtype=np.float32)
self._inited = False
self._is_verify = is_verify
Copy link
Collaborator

Choose a reason for hiding this comment

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

is_verify --> to_verify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

class CriticalException(Exception):
pass


class SharedNDArray(object):
"""SharedNDArray utilizes shared memory to avoid data serialization when
object of which shared between different processes. We can reconstruct the
Copy link
Collaborator

Choose a reason for hiding this comment

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

object of which ---> data object
between --> among

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


Args:
name (str): Address name of shared memory.
is_verify (bool): Whether to do validation for writing operation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is_verify --> to_verify
Whether to do validation for writing operation --> Whether to validate the writing operation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@kuke kuke left a comment

Choose a reason for hiding this comment

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

LGTM

@pkuyym pkuyym merged commit c16058e into PaddlePaddle:develop Mar 13, 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.

None yet

3 participants