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.

Sign up for GitHub

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

Jump to bottom

DOCS-#88: Update main/getting_started pages of rst docs #93

Merged
merged 5 commits into from
Jan 27, 2022
Merged

DOCS-#88: Update main/getting_started pages of rst docs #93

merged 5 commits into from
Jan 27, 2022

Conversation

prutskov
Copy link
Contributor

@prutskov prutskov commented Jan 19, 2022 •
edited
Loading

Signed-off-by: Alexey Prutskov alexey.prutskov@intel.com

What do these changes do?

@prutskov prutskov marked this pull request as draft January 19, 2022 16:21
@YarShev YarShev added the Blocked ❌ A pull request that is blocked label Jan 19, 2022
@prutskov prutskov mentioned this pull request Jan 21, 2022
Merged
6 tasks
@YarShev YarShev removed the Blocked ❌ A pull request that is blocked label Jan 21, 2022
@YarShev
Copy link
Collaborator

YarShev commented Jan 21, 2022

@prutskov, when we were discussing this task on our meeting, we were talking about getting started page as well. Can you update scope of the issue and introduce related changes to this PR?

@prutskov
Copy link
Contributor Author

@prutskov, when we were discussing this task on our meeting, we were talking about getting started page as well. Can you update scope of the issue and introduce related changes to this PR?

Changes for Getting Started is in progress (that's reason why this PR is in draft). The issue scope will be updated.

@YarShev
Copy link
Collaborator

YarShev commented Jan 21, 2022

@prutskov, when we were discussing this task on our meeting, we were talking about getting started page as well. Can you update scope of the issue and introduce related changes to this PR?

Changes for Getting Started is in progress (that's reason why this PR is in draft). The issue scope will be updated.

Okay, thanks!

@prutskov
DOCS-modin-project#88: Update main/getting_started pages of rst docs
5a051ba 
Signed-off-by: Alexey Prutskov <alexey.prutskov@intel.com>
@prutskov prutskov changed the title DOCS-#88: Update rst docs start page DOCS-#88: Update main/getting_started pages of rst docs Jan 25, 2022
@prutskov prutskov marked this pull request as ready for review January 25, 2022 16:04
YarShev
YarShev reviewed Jan 26, 2022
View reviewed changes
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst

# Get materialized data.
print(unidist.get(refs)) # [0, 1, 4, 9]
square_refs = [square.remote(i) for i in range(4)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is getting results?

Copy link
Contributor Author

@prutskov prutskov Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is placed after cubes computing. There is no get here to highlight that references could be provided in the next remote function/actor method without forcing computing using get.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see getting materialized data on square_refs.

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'm talking about this above. There is get only for cubes_refs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is strange to me that we only show materializing data cubes_refs. Let's add it for square_refs too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was not for me. Than why did you want to call get on square_refs if they are already materialized through chaining?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Those materialized in a worker but not in the main process.

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 added unidist.get(square_refs) call before unidist.get(cude_refs). I think @YarShev only wants to demonstrate, additionally, what data are placed behind square_refs .

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just to add comment that you can call get on square_refs but also to implement chaining?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Chaining is shown for Cube, getting the data further. I think it is okay.

YarShev
YarShev reviewed Jan 26, 2022
View reviewed changes
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
no-ponomarev
no-ponomarev reviewed Jan 26, 2022
View reviewed changes
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@YarShev
Copy link
Collaborator

YarShev commented Jan 27, 2022

@prutskov, just in case, you can add suggestions to batch to be applied at once in order not to commit each separate suggestion. That help to avoid multiple email messages.

@prutskov
Copy link
Contributor Author

@prutskov, just in case, you can add suggestions to batch to be applied at once in order not to commit each separate suggestion. That help to avoid multiple email messages.

This is clear. But sometimes some of suggestions could be applied at once, but others can be missed/merged later after investigating. You also can setting up email-notifications/subscriptions. We haven't something restrictions on amount of commits in own branches during developing process.

YarShev
YarShev approved these changes Jan 27, 2022
View reviewed changes
Copy link
Collaborator

@YarShev YarShev left a comment

Choose a reason for hiding this comment

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

@prutskov, LGTM, thanks!

@prutskov
Copy link
Contributor Author

@no-ponomarev, If you don't have additional comments, merge this PR. get_ip failure isn't a stopper in our case (#103 (comment))

@no-ponomarev no-ponomarev merged commit 3cf5720 into modin-project:master Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@no-ponomarev no-ponomarev no-ponomarev left review comments

@YarShev YarShev YarShev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update main/getting_started pages of rst docs
3 participants
@prutskov @YarShev @no-ponomarev