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

Add new resource hint to all sdks for number of cpus per worker machine #28848

Merged
merged 10 commits into from
Oct 12, 2023

Conversation

kerrydc
Copy link
Contributor

@kerrydc kerrydc commented Oct 5, 2023

This resource hint allows for further specifcation of user needs at the transfor or pipeline level.

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 or the workflows README to see a list of phrases to trigger workflows.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #28848 (ddf272b) into master (31e1c7a) will decrease coverage by 0.09%.
Report is 87 commits behind head on master.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##           master   #28848      +/-   ##
==========================================
- Coverage   72.23%   72.15%   -0.09%     
==========================================
  Files         684      686       +2     
  Lines      101198   101613     +415     
==========================================
+ Hits        73102    73317     +215     
- Misses      26518    26716     +198     
- Partials     1578     1580       +2     
Flag Coverage Δ
go 53.44% <87.50%> (+0.02%) ⬆️
python 82.58% <85.71%> (-0.21%) ⬇️

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

Files Coverage Δ
sdks/python/apache_beam/transforms/resources.py 92.56% <85.71%> (-0.43%) ⬇️
sdks/go/pkg/beam/options/resource/hint.go 90.32% <87.50%> (-4.49%) ⬇️

... and 28 files with indirect coverage changes

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @AnandInguva for label python.
R: @Abacn for label java.
R: @jrmccluskey for label go.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

sdks/python/.python-version Outdated Show resolved Hide resolved
sdks/python/apache_beam/transforms/resources.py Outdated Show resolved Hide resolved
Copy link
Contributor

@AnandInguva AnandInguva left a comment

Choose a reason for hiding this comment

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

LGTM!! Python changes look good to me

Copy link
Contributor

@Abacn Abacn left a comment

Choose a reason for hiding this comment

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

thanks, LGTM from java side. Just had a minor comment about documentation

Copy link
Contributor

@jrmccluskey jrmccluskey left a comment

Choose a reason for hiding this comment

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

LGTM from the Go side

@Abacn
Copy link
Contributor

Abacn commented Oct 10, 2023

there are format errors, for java, run ./gradlew :sdks:java:core:spotlessApply

for python, lint error not related to this PR

@Abacn
Copy link
Contributor

Abacn commented Oct 10, 2023

There is go test error:

# github.com/apache/beam/sdks/v2/go/pkg/beam/options/resource [github.com/apache/beam/sdks/v2/go/pkg/beam/options/resource.test]
Error: pkg/beam/options/resource/hint_test.go:167:30: undefined: pipepb.StandardResourceHints_CPU_COUNT

@lostluck
Copy link
Contributor

LGTM for the Updated Go code, if we get the Go Tests precommit to pass.

@Abacn
Copy link
Contributor

Abacn commented Oct 11, 2023

it appears go test is failing on master

image

However the first a few red run was not go change.

EDIT: actually this is an long standing flaky test, opened #28951

@Abacn
Copy link
Contributor

Abacn commented Oct 12, 2023

Run Java_PVR_Flink_Batch PreCommit

@Abacn
Copy link
Contributor

Abacn commented Oct 12, 2023

Run Java PreCommit

@Abacn
Copy link
Contributor

Abacn commented Oct 12, 2023

java precommit timeout due to #28957 not related - merging for now

@Abacn Abacn merged commit 4a7c484 into apache:master Oct 12, 2023
104 of 107 checks passed
damondouglas pushed a commit to damondouglas/beam that referenced this pull request Oct 16, 2023
…ne (apache#28848)

* Adds new resource hint for number of cpus per worker.

* Fixes minor bugs.`

* Go fmt and removes unwanted .python-version file.

* Adds tests.

* Fixes typo.

* Fixes Java tests, adds URN to parsers.

* Addresses FindBugs issue with int parsing.

* Applies Java formatting corrections

* Adds generated go protobufs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants