Skip to content

Conversation

kboroszko
Copy link
Collaborator

@kboroszko kboroszko commented Nov 24, 2021

Docstrings for API and jupyter notebook with quickstart


This change is Reviewable

@kboroszko kboroszko requested a review from dopiera November 24, 2021 16:06
@kboroszko kboroszko closed this Nov 24, 2021
@kboroszko kboroszko reopened this Nov 24, 2021
Copy link

@dopiera dopiera left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 15 files reviewed, 6 unresolved discussions (waiting on @dopiera and @kboroszko)


tensorflow_io/core/kernels/bigtable/bigtable_dataset_kernel.cc, line 136 at r1 (raw file):

mutex mu_;

Please remove this mutex (and the corresponding annotations). Also, I think this got here by accident, no?


tensorflow_io/python/ops/bigtable/bigtable_dataset_ops.py, line 43 at r1 (raw file):

class BigtableTable:
    """Entry point for reading data from Cloud Bigtable.

As much as this description is accurate, I think the user will be interested in what it is rather than what it does under the hood.

Perhaps rephrasing it slightly to indicate that this object represents a table in Bigtable is enough? Up to you, though.


tensorflow_io/python/ops/bigtable/bigtable_dataset_ops.py, line 66 at r1 (raw file):

        output_type=tf.string,
    ):
        """Retrieves values from Google Bigtable.

Please indicate that results will be sorted by row key.


tensorflow_io/python/ops/bigtable/bigtable_dataset_ops.py, line 94 at r1 (raw file):

        output_type=tf.string,
    ):
        """Retrieves values from Google Bigtable in parallel. The ammount of work is split between workers

Please indicate that results will not be sorted by row key.


tensorflow_io/python/ops/bigtable/bigtable_row_range.py, line 21 at r1 (raw file):

class RowRange:
    """Python wrapper for google::cloud::bigtable::RowRange"""

The fact that it is a wrapper is an internal detail. How about we simply say that it represents a continuous range of rows.


tensorflow_io/python/ops/bigtable/bigtable_row_set.py, line 23 at r1 (raw file):

    """Python wrapper for google::cloud::bigtable::RowSet"""

Same as with RowRange, please write the docstring such that it describes what it is rather than how it's implemented.

Copy link
Collaborator Author

@kboroszko kboroszko left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 15 files reviewed, 6 unresolved discussions (waiting on @dopiera)


tensorflow_io/core/kernels/bigtable/bigtable_dataset_kernel.cc, line 136 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…
mutex mu_;

Please remove this mutex (and the corresponding annotations). Also, I think this got here by accident, no?

It did. Sneaky little mutex. I wanted this PR to focus only on docs and python API, so I won't touch the c++ code here and remove this change as well. It will be a part of a different PR.


tensorflow_io/python/ops/bigtable/bigtable_dataset_ops.py, line 43 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

As much as this description is accurate, I think the user will be interested in what it is rather than what it does under the hood.

Perhaps rephrasing it slightly to indicate that this object represents a table in Bigtable is enough? Up to you, though.

Done.


tensorflow_io/python/ops/bigtable/bigtable_dataset_ops.py, line 66 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

Please indicate that results will be sorted by row key.

Done.


tensorflow_io/python/ops/bigtable/bigtable_dataset_ops.py, line 94 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

Please indicate that results will not be sorted by row key.

Done.


tensorflow_io/python/ops/bigtable/bigtable_row_range.py, line 21 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

The fact that it is a wrapper is an internal detail. How about we simply say that it represents a continuous range of rows.

Done.


tensorflow_io/python/ops/bigtable/bigtable_row_set.py, line 23 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…
    """Python wrapper for google::cloud::bigtable::RowSet"""

Same as with RowRange, please write the docstring such that it describes what it is rather than how it's implemented.

Done.

Copy link

@dopiera dopiera left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 15 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dopiera)

@kboroszko kboroszko merged commit 7b97049 into master Nov 25, 2021
@kboroszko kboroszko deleted the kb/docs branch November 25, 2021 11:54
dopiera pushed a commit that referenced this pull request Dec 3, 2021
dopiera pushed a commit that referenced this pull request Dec 3, 2021
kboroszko added a commit that referenced this pull request Dec 13, 2021
kboroszko added a commit that referenced this pull request Dec 20, 2021
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