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

[Go SDK] Dataframe API wrapper #23450

Merged
merged 9 commits into from
Oct 18, 2022
Merged

[Go SDK] Dataframe API wrapper #23450

merged 9 commits into from
Oct 18, 2022

Conversation

riteshghorse
Copy link
Contributor

@riteshghorse riteshghorse commented Sep 30, 2022

Wrapped Dataframe API with integration test. This needs beam python container later than 2.41.0 (because it needs RowsToDataFrameFn internally which was added after 2.41.0) so tested it with 2.42.0rc1.

Closes #23384


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.

@github-actions github-actions bot added the go label Sep 30, 2022
@riteshghorse
Copy link
Contributor Author

riteshghorse commented Sep 30, 2022

Should not be merged until after #23383 , tests will likely fail on this PR until then.

@riteshghorse
Copy link
Contributor Author

R: @lostluck

@github-actions
Copy link
Contributor

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

@riteshghorse riteshghorse changed the title [Go SDK Dataframe api wrapper for GO SDK [Go SDK Dataframe API wrapper for GO SDK Sep 30, 2022
@riteshghorse riteshghorse changed the title [Go SDK Dataframe API wrapper for GO SDK [Go SDK] Dataframe API wrapper for GO SDK Sep 30, 2022
@riteshghorse riteshghorse changed the title [Go SDK] Dataframe API wrapper for GO SDK [Go SDK] Dataframe API wrapper Sep 30, 2022
@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Merging #23450 (b7b7136) into master (107a43d) will increase coverage by 0.09%.
The diff coverage is n/a.

❗ Current head b7b7136 differs from pull request most recent head e68ca5b. Consider uploading reports for the commit e68ca5b to get more accurate results

@@            Coverage Diff             @@
##           master   #23450      +/-   ##
==========================================
+ Coverage   73.35%   73.44%   +0.09%     
==========================================
  Files         719      718       -1     
  Lines       95800    95884      +84     
==========================================
+ Hits        70276    70424     +148     
+ Misses      24212    24149      -63     
+ Partials     1312     1311       -1     
Flag Coverage Δ
go 50.87% <0.00%> (-0.09%) ⬇️
python 83.20% <0.00%> (+0.15%) ⬆️

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

Impacted Files Coverage Δ
sdks/go/pkg/beam/runners/direct/gbk.go 72.35% <0.00%> (-11.24%) ⬇️
.../python/apache_beam/testing/test_stream_service.py 88.09% <0.00%> (-4.77%) ⬇️
sdks/python/apache_beam/internal/gcp/auth.py 78.66% <0.00%> (-4.20%) ⬇️
sdks/go/pkg/beam/runners/direct/direct.go 65.76% <0.00%> (-2.71%) ⬇️
sdks/python/apache_beam/utils/interactive_utils.py 95.12% <0.00%> (-2.44%) ⬇️
...che_beam/runners/interactive/interactive_runner.py 90.50% <0.00%> (-1.27%) ⬇️
sdks/go/pkg/beam/core/runtime/harness/harness.go 10.00% <0.00%> (-1.12%) ⬇️
...ks/python/apache_beam/runners/worker/statecache.py 91.86% <0.00%> (-1.07%) ⬇️
...ache_beam/runners/dataflow/ptransform_overrides.py 90.12% <0.00%> (-0.79%) ⬇️
...hon/apache_beam/runners/direct/test_stream_impl.py 93.28% <0.00%> (-0.75%) ⬇️
... and 13 more

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

@lostluck
Copy link
Contributor

lostluck commented Oct 4, 2022

PR #23383 is now merged, so ready for re-testing.

I still need to look at the code, but please also add a mention of Dataframes to CHANGES.md

@riteshghorse
Copy link
Contributor Author

The GoPortable failures are because of TestDataframe test. Currently, it is trying to expand it on the java expansion service. Need to plumb the python expansion service with python port from here

def goTask = project.project(":sdks:go:test:").goIoValidatesRunnerTask(project, config.name+"GoUsingJava", config.goScriptOptions, pipelineOpts)

@riteshghorse
Copy link
Contributor Author

riteshghorse commented Oct 5, 2022

For the current PR, filtered the test if python_transform address is not provided for expansion service.
Filed: #23503

@riteshghorse
Copy link
Contributor Author

Run Go PostCommit

@riteshghorse
Copy link
Contributor Author

Run XVR_Flink PostCommit

@riteshghorse
Copy link
Contributor Author

The error in postcommit is due to the insufficient quota in apache-beam-testing as we saw in earlier PR

@riteshghorse
Copy link
Contributor Author

Run Go PostCommit

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

Only one comment, then I'll merge this in. Thanks for your patience!

sdks/go/pkg/beam/transforms/xlang/python/external.go Outdated Show resolved Hide resolved
@lostluck
Copy link
Contributor

Don't forget to resolve the CHANGES.md conflicts too.

@lostluck lostluck merged commit 78e1c0a into apache:master Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request][Go SDK]: Dataframe API wrapper for Go SDK
2 participants