Skip to content

Add new resource hints: max active bundles per worker #34529

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

Merged
merged 15 commits into from
Apr 22, 2025

Conversation

AMOOOMA
Copy link
Contributor

@AMOOOMA AMOOOMA commented Apr 2, 2025

Adding SDK support for passing in max active bundles per worker as new resource hints.

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.

@AMOOOMA AMOOOMA changed the title Add new resource hints: max active dofn per worker Add new resource hints: max active bundles per worker Apr 14, 2025
@AMOOOMA AMOOOMA marked this pull request as ready for review April 15, 2025 16:34
Copy link
Contributor

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

R: @jrmccluskey for label python.
R: @shunping for label java.
R: @lostluck 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).

@lostluck
Copy link
Contributor

lostluck commented Apr 15, 2025

The Go changes LGTM, though I must remind that WRT the model, hints are only intended for performance, not correctness. That is, Ressource hints that a runner doesn't know, must be able to be ignored for correctness.

For this one, I'd be most worried about trying to force single threading for such a bundle communicating to a GPU. On the other hand "Please run as many of these as the worker can handle eg. 100" would be very safe. Really depends on the intent. I got lost about what the name of the hint was and over speculated. This is at least precise in the intent despite the concern I have above.

Copy link
Collaborator

@shunping shunping left a comment

Choose a reason for hiding this comment

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

The Java code change looks good. Just one minor comment.

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.

Python piece LGTM

@shunping
Copy link
Collaborator

Thanks! Looks good to me as well.

@jrmccluskey jrmccluskey merged commit b7bd127 into apache:master Apr 22, 2025
116 of 117 checks passed
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.

4 participants