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

Issue#21430 Avoid pruning DataframeTransforms #23069

Merged
merged 3 commits into from
Sep 16, 2022
Merged

Issue#21430 Avoid pruning DataframeTransforms #23069

merged 3 commits into from
Sep 16, 2022

Conversation

nika-qubit
Copy link
Contributor

@nika-qubit nika-qubit commented Sep 7, 2022 •
edited
Loading

Fixes #21430.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@nika-qubit
Issue#21430 Avoid pruning DataframeTransforms
1df13d3 
Avoided the pruning of composite transforms for Beam dataframe.
@nika-qubit
Copy link
Contributor Author

Tested merging Beam DataFrames.
BhbUabtPYLKZf2J

To workaround for #22587

R: @PhilippeMoussalli
R: @TheNeuralBit

PTAL, thx!

@nika-qubit nika-qubit mentioned this pull request Sep 7, 2022
Merged
2 tasks
@codecov
Copy link

codecov bot commented Sep 7, 2022 •
edited
Loading

Codecov Report

Merging #23069 (d33931a) into master (0d937d4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #23069      +/-   ##
==========================================
+ Coverage   73.58%   73.59%   +0.01%     
==========================================
  Files         716      716              
  Lines       95301    95331      +30     
==========================================
+ Hits        70127    70159      +32     
+ Misses      23878    23876       -2     
  Partials     1296     1296
Flag Coverage Δ
python 83.41% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ache_beam/runners/interactive/pipeline_fragment.py 99.18% <100.00%> (+0.85%) ⬆️
...pache_beam/runners/interactive/testing/mock_env.py 97.56% <100.00%> (+0.06%) ⬆️
...s/python/apache_beam/io/gcp/bigquery_file_loads.py 87.24% <0.00%> (-0.47%) ⬇️
...beam/runners/portability/local_job_service_main.py 14.43% <0.00%> (-0.42%) ⬇️
sdks/python/apache_beam/io/gcp/bigquery.py 74.24% <0.00%> (ø)
...thon/apache_beam/ml/inference/pytorch_inference.py 0.00% <0.00%> (ø)
...beam/runners/portability/expansion_service_main.py 0.00% <0.00%> (ø)
...am/examples/inference/pytorch_language_modeling.py 0.00% <0.00%> (ø)
...hon/apache_beam/runners/worker/bundle_processor.py 93.54% <0.00%> (+0.12%) ⬆️
sdks/python/apache_beam/transforms/trigger.py 90.13% <0.00%> (+0.26%) ⬆️
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot added the python label Sep 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

TheNeuralBit
TheNeuralBit reviewed Sep 13, 2022
View reviewed changes
Copy link
Member

@TheNeuralBit TheNeuralBit left a comment

Choose a reason for hiding this comment

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

A couple of other questions:

  • Could we make the manual test you did into a unit test?
  • I think we should file a follow-up issue to enable pruning within dataframe transform

sdks/python/apache_beam/runners/interactive/pipeline_fragment.py
def should_skip_pruning(transform: AppliedPTransform):
return (
isinstance(transform.transform, TestStream) or
'_DataFrame_' in transform.full_label)
Copy link
Member

Choose a reason for hiding this comment

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

can we match on is instance DataframeTransform instead of string matching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DataframeTransform expands into a bunch of normal ParDos, so DataframeTransform itself doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, if there's a minimally invasive change we can make in the DataFrame API that would allow this filter be more selective, I'd be in favor of making that here. But that doesn't need to be a blocker.

Copy link
Member

Choose a reason for hiding this comment

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

Also - it might be good to make sure this works for operations on DeferredSeries objects as well (i.e. do we need to match on "_Series"?). This could be challenging though. I don't think we've encountered an example of a series getting pruned.

Copy link
Contributor Author

@nika-qubit nika-qubit Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would recommend adding unittests with ib.collect for each Series operation. In that case, we don't have to skip pruning prematurely.

The same also applies to the DeferredDataFrame operations. If applicable (when a proper pruning is in-place, in the future add some unittest with ib.collect (following the InteractiveDataFrameTest in this PR).

@nika-qubit
Copy link
Contributor Author

A couple of other questions:

  • Could we make the manual test you did into a unit test?
    Yes, let me write this into a unit test.
  • I think we should file a follow-up issue to enable pruning within dataframe transform

Let's keep the issue open but modify the description for proper pruning.
We might just need to skip the pruning for some specific composite transforms instead of all DataFrame-created ones.

TheNeuralBit
TheNeuralBit approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@TheNeuralBit TheNeuralBit left a comment

Choose a reason for hiding this comment

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

LGTM, once there's a test passing in CI

@nika-qubit
Copy link
Contributor Author

Retest this please.

@nika-qubit
Copy link
Contributor Author

Run Python_PVR_Flink PreCommit

@nika-qubit
Copy link
Contributor Author

All tests have passed. One of the tests shows pending but has actually passed.

@nika-qubit nika-qubit merged commit b03dd60 into apache:master Sep 16, 2022
@TheNeuralBit TheNeuralBit mentioned this pull request Sep 16, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheNeuralBit TheNeuralBit TheNeuralBit approved these changes

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

Successfully merging this pull request may close these issues.

DataFrame components cannot be pruned by Interactive Beam
2 participants
@nika-qubit @TheNeuralBit