Skip to content

[DataFrame] Fix blocking issue on _IndexMetadata passing #1965

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
Merged
devin-petersohn merged 8 commits into from
May 2, 2018
Merged

[DataFrame] Fix blocking issue on _IndexMetadata passing #1965

devin-petersohn merged 8 commits into from
May 2, 2018

Conversation

@p-yang
Copy link
Contributor

@p-yang p-yang commented Apr 28, 2018

What do these changes do?

Fixes a performance issue on passing _IndexMetadata objects for applymap and related functions that don't mutate indexes.

@AmplabJenkins
Copy link

AmplabJenkins commented Apr 28, 2018

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5108/
Test PASSed.

@AmplabJenkins
Copy link

AmplabJenkins commented Apr 28, 2018

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5112/
Test FAILed.

devin-petersohn
devin-petersohn reviewed Apr 29, 2018
View reviewed changes
Copy link
Member

@devin-petersohn devin-petersohn left a comment

Choose a reason for hiding this comment

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

It would be great to get numbers on the performance difference here.

python/ray/dataframe/dataframe.py Outdated
Copy link
Member

@devin-petersohn devin-petersohn Apr 29, 2018

Choose a reason for hiding this comment

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

Are you planning to do this on this PR?

python/ray/dataframe/dataframe.py Outdated
Copy link
Member

@devin-petersohn devin-petersohn Apr 29, 2018

Choose a reason for hiding this comment

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

What is the bp in bp_length?

python/ray/dataframe/dataframe.py Outdated
Copy link
Member

@devin-petersohn devin-petersohn Apr 29, 2018

Choose a reason for hiding this comment

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

can we get rid of this line since we only need the metadata now?

Copy link
Contributor Author

@p-yang p-yang Apr 29, 2018

Choose a reason for hiding this comment

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

We can't in this case, as discussed today. If we don't pass the columns then the metadata of the new dataframe won't have the column changes reflected. Therefore, we need to either copy the metadata and modify the copy and push the copy, or pass the new columns such that the constructor modifies the metadata object copy on its end.

python/ray/dataframe/dataframe.py Outdated
Copy link
Member

@devin-petersohn devin-petersohn Apr 29, 2018

Choose a reason for hiding this comment

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

Same as above, can we get rid of this line?

Copy link
Contributor Author

@p-yang p-yang Apr 29, 2018

Choose a reason for hiding this comment

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

Same as above.

@devin-petersohn
Copy link
Member

devin-petersohn commented Apr 29, 2018

Follow-up question: Can we make it so that index is cached in this PR?

@p-yang
Copy link
Contributor Author

p-yang commented Apr 29, 2018

Reference on performance numbers (these are off the top of my head, and testing on c69 yielded results with high variance):

On c69 limited to 8 ray workers, 5GB int64 CSV df.isna:

  1. With 1x6 partitions: pre-change 300ms, post-change 182ms
  2. With 2x6 partitions: pre-change 600ms, post-change 186ms

@AmplabJenkins
Copy link

AmplabJenkins commented Apr 29, 2018

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5118/
Test PASSed.

@AmplabJenkins
Copy link

AmplabJenkins commented Apr 30, 2018

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5119/
Test PASSed.

devin-petersohn
devin-petersohn reviewed Apr 30, 2018
View reviewed changes
Copy link
Member

@devin-petersohn devin-petersohn left a comment

Choose a reason for hiding this comment

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

These changes look really great! Just a couple of minor quick comments.

python/ray/dataframe/utils.py Outdated
Copy link
Member

@devin-petersohn devin-petersohn Apr 30, 2018

Choose a reason for hiding this comment

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

Add doc """Compute widths for each partition."""

python/ray/dataframe/index_metadata.py Outdated
Copy link
Member

@devin-petersohn devin-petersohn Apr 30, 2018

Choose a reason for hiding this comment

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

Are you planning to add this fix in this PR?

Copy link
Contributor Author

@p-yang p-yang Apr 30, 2018

Choose a reason for hiding this comment

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

This was actually silently fixed by the other changes, setting a new DF will implicitly change the index on the metadata object as well. Comment removed.

python/ray/dataframe/index_metadata.py Outdated
Copy link
Member

@devin-petersohn devin-petersohn Apr 30, 2018

Choose a reason for hiding this comment

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

Could you write a comment about how to use __getitem__? We have had people using it in incorrect ways and trying to make it work for them, so this way hopefully we can avoid that.

@p-yang p-yang mentioned this pull request May 1, 2018
Merged
@devin-petersohn
Copy link
Member

devin-petersohn commented May 2, 2018

Jenkins, retest this please

@AmplabJenkins
Copy link

AmplabJenkins commented May 2, 2018

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5141/
Test PASSed.

@devin-petersohn devin-petersohn merged commit 5589426 into ray-project:master May 2, 2018
@devin-petersohn
Copy link
Member

devin-petersohn commented May 2, 2018

Merged, thanks @Veryku

@devin-petersohn devin-petersohn mentioned this pull request May 2, 2018
Closed
alok added a commit to alok/ray that referenced this pull request May 6, 2018
@alok
Merge branch 'magic-methods' into update-a3c-torch
1da4a7a 
* magic-methods:
  fmt
  Fix IndentationError
  Write magic methods for SampleBatch/PartialRollout
  Clean up syntax for supported Python versions. (ray-project#1963)
  [DataFrame] Implements mode, to_datetime, and get_dummies (ray-project#1956)
  [DataFrame] Fix dtypes (ray-project#1930)
  keep_dims -> keepdims (ray-project#1980)
  add pthread linking (ray-project#1986)
  [DataFrame] Add layer of abstraction to allow OID instantiation (ray-project#1984)
  [DataFrame] Fix blocking issue on _IndexMetadata passing (ray-project#1965)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devin-petersohn devin-petersohn devin-petersohn 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.

3 participants

@p-yang @AmplabJenkins @devin-petersohn