-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-45527][CORE] Use fraction to do the resource calculation #44690
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
Closed
Closed
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
6d9ea01
[SPARK-45527][CORE] Use fraction to do the resource calculation
wbo4958 787e876
fix mesos issue
wbo4958 66b2d3f
unicode
wbo4958 b58c030
fix mesos test failing issue
wbo4958 5196f75
Merge remote-tracking branch 'upstream/branch-3.5' into fraction-gpu
wbo4958 2aafdf5
Merge remote-tracking branch 'upstream/branch-3.5' into fraction-gpu
wbo4958 77fbae7
Merge remote-tracking branch 'upstream/branch-3.5' into fraction-gpu
wbo4958 aa4f62f
a minor improvement
wbo4958 0e47f91
[SPARK-46721][CORE][TESTS] Make gpu fraction tests more robust
wbo4958 5443184
Merge remote-tracking branch 'upstream/branch-3.5' into fraction-gpu
wbo4958 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way too hacky.
I get it, but, the right solution in your example is to ask for something like 0.111 GPUs to get 9. This works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @srowen,
Thank you for your review. You've suggested a straightforward solution to address the issue of double precision by actively sacrificing some accuracy, and it's certainly doable. However, this raises a question about the precision that Spark should define. For instance, if a user sets the value to 0.1111111111111, which value should Spark use for the calculation: 0.111, 0.1111, or something else? If we simply select 4 decimal places, what happens if a user wants to set the value to 0.000001? In that case, the value would be converted to 0 and might raise an exception. But It seems that a configuration option could resolve this problem.
My initial thought was that the resource fraction calculation should work regardless of the value set by the user, and the proposed approach in the pull request to convert the double to a Long can achieve this goal.
Nevertheless, if you and other committers believe that a change is necessary, I am open to submitting a pull request for the master branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it also doesn't feel wholly satisfying. In this example all of those values work as 9 times even 0.11 leaves you with less than 0.11 remaining, so you schedule 9. It would also imply there is 0.01 GPU left when that isn't the intent. In practice, I strongly doubt anyone is ever scheduling, let's say, more than 100 tasks on one GPU.
(But what about non-GPU resources? there aren't any now. Are there resources you'd schedule very very small fractions of? I can't think of any even in the future.)
Going down the rabbit hole of floating-point precision, I think we hit that no matter what. If I ask for 1e-16 resources, any way we interpret that probably is slightly imprecise as it's interpreted as float somewhere. But these are unrealistic use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srowen what if your major concern here? Are you seeing performance issues? just more complex then you think is necessary? i don't think its hacky, it might be overkill but I do agree its likely more then we need for precision. I would go more then 100 though as 100 tasks per some resource seems unlikely but not impossible, 1000 - 10000 much more unlikely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can solve floating-point accuracy here in the general case, and this will virtually never arise anyway, except in one important class of case -- n GPUs where n < 10 and n is relatively prime to 10. Like, 3 even. A person writing down the resource utilization will almost surely write "0.333", but one can imagine supplying
str(1./3.)programmatically. And then this issue could arise. Some string like "0.333333333" may end up as a float that has a value just over 1/3.The other issue is approaching it this way by just multiplying by a long. I guess instead this can be done properly with BigDecimal at least? and I don't think this should touch so many parts of the code. Surely this just affects how the resource request string is parsed and compared to available resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @srowen,
The using Long way is quite the same with the BigDecimal. The default scale of BigDecimal is 16, so this PR chooses (ONE_ENTIRE_RESOURCE = 1E16.toLong)
So if we need to ensure the input is small enough (<1/n) and we can set the scale to be like 14 for BigDecimal, and similarly, to keep align with BigDecimal, we can set
ONE_ENTIRE_RESOURCE = 1E14.toLongThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The point of Big decimal is that you never have to use double or float. This isn't actually doing math with BD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @srowen. Actually, this PR converts the taskAmount which is a double/float value to a Long by multiplying
1E16.toLong, and then the following calculation is based on Long instead of double/float. you can see, all the APIs of ExecutorResourcesAmount are using Long. Even the assigned resource of a task is still keeping the Long, you can refer to there.Yeah, but you think we should use BigDecimal, I'm Okay for that, I can make a PR for master branch first, and then cherry-pick to 3.5 branch.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. Above I give an example where multiplying by long doesn't work. I'm referring to your example of BigDecimal above, which does not use (only) BigDecimal. Please just try it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @srowen. Sure, let me have a PR using BD for master branch. Thx