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

[DOC-REVIEW] Frames README (#2) #292

Merged
merged 8 commits into from
Sep 29, 2019

Conversation

Sharon-iguazio
Copy link
Contributor

No description provided.

Copy link
Contributor

@talIguaz talIguaz left a comment

Choose a reason for hiding this comment

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

Finally the docs will look good 💪

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
<a id="method-create-params-tsdb"></a>
#### tsdb Backend create Parameters

- <a id="method-create-tsdb-param-rate"></a>**rate** (Required) &mdash; `string` &mdash; the ingestion rate TSDB's metric-samples, as `"[0-9]+/[smh]"` (where `s` = seconds, `m` = minutes, and `h` = hours); for example, `"1/s"` (one sample per minute).
Copy link
Contributor

Choose a reason for hiding this comment

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

... the approximate ingestion rate for the TSDB's metric-samples ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talIguaz the next line in this parameter description is "The rate should be calculated according to the slowest expected ingestion rate.". Doesn't this cover what you wanted to add?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think slowest is necessarily better. i guess an average rate will suite better.
But it depends on if there's a big difference between the expected rates. if so we need to think what will be better, otherwise i guess avg should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talIguaz currently, in our doc-site TSDB CLI tutorial, Frames v3io/tutorials NB, and Frames README file we say that the TSDB ingestion rate should be set according to the slowest expected ingestion rate. This doc was added based on Golan's guidelines. @dinal please let me know whether we need to change to this guideline, in light of Tal's input, and if so, what do you want the new guideline to be.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Sharon-iguazio
Copy link
Contributor Author

@talIguaz I finished the second review round and committed the changes to the PR. In addition to changes pertaining to your review comments, I also made additional fixes and improvements — including to add return-value sections for the client constructor and the read method, change the syntax code snippets, and fix and improve the method parameter descriptions (to be continued ...).
I also edited the help text in the client code files (again, only the first round).

session : Session
Session object
frame_factory : class
DataFrame factory (currencly pandas and cudf supported)
DataFrame factory; currently, pandas and cuDF are supported
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we want to say that cuDF is officially supported yet

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 agree. @talIguaz @dinal feel free to remove the cuDF reference from the help text; note that it's not included in the README. (As I didn't write the original help text, I didn't want to take it upon myself to remove the cuDF reference from this text and I didn't have time to ask last week.)


All Frames backends that support the `read` method support the following common parameters:

- <a id="method-read-param-iterator"></a>**iterator** &mdash; (Optional) (default: `False`) &mdash; `bool` &mdash; `True` to return a DataFrames iterator; `False` to return a single DataFrame.
Copy link
Contributor

Choose a reason for hiding this comment

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

better to order the parameters with the optional ones in the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talIguaz I plan to reorder all parameter lists alphabetically and include separate requirement bullets (as I did in the constructor documentation). I just didn't have time to handle this for this PR + I want to do the reordering in a separate commit to simplify diffs.


- <a id="method-create-param-attrs"></a>**attrs** &mdash; A dictionary of `<argument name>: <value>` pairs for passing additional backend-specific parameters (arguments).

- **Type:** dict
Copy link
Contributor

Choose a reason for hiding this comment

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

The style of the parameter description here is different than the other methods (read,write..). is it on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talIguaz ultimately, all parameter descriptions should have separate type and requirement bullets (and valid and default values bullets where relevant), I just didn't have time to do it for this PR.

@dinal dinal merged commit 83c3712 into v3io:development Sep 29, 2019
@Sharon-iguazio Sharon-iguazio deleted the doc-review-frames-2 branch October 2, 2019 16:55
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