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

Apply Operand Closure clean up #3205

Merged
merged 16 commits into from
Aug 24, 2022
Merged

Apply Operand Closure clean up #3205

merged 16 commits into from
Aug 24, 2022

Conversation

vcfgv
Copy link
Contributor

@vcfgv vcfgv commented Aug 4, 2022 •
edited
Loading

What do these changes do?

  1. Compute logic key and for specific operands before tiling.
  2. While tiling, put entire UDF to storage when sum of sizes of cell_contents exceed a certain threshold, which is customizable for users to adjust or disable this feature.
  3. While executing, get UDF from storage and restore to op.func.
  4. Add storage service to supervisor to avoid store func on workers who may risk being released.

Under ray task mode, func key, i.e. ray.ObjectRef, shall be serialized by Ray rather than Mars. As a result, relevant tests are temporarily disabled for Github checks but passed locally with some modifications.

Lifecycle management of UDF storage object is planned in another PR.

Related issue number

#3193

Check code requirements

  • tests added / passed (if needed)
  • Ensure all linting tests pass, see here for how to run them

fyrestone
fyrestone reviewed Aug 4, 2022
View reviewed changes
mars/services/context.py Outdated Show resolved Hide resolved
mars/dataframe/base/apply.py Outdated Show resolved Hide resolved
@fyrestone
Copy link
Contributor

Some suggestions are here: #3193 (comment)

@vcfgv vcfgv force-pushed the closure_clean_up branch 4 times, most recently from 126a14c to bd40696 Compare August 10, 2022 02:55
@vcfgv vcfgv force-pushed the closure_clean_up branch from bd40696 to aedddb9 Compare August 10, 2022 03:21
@vcfgv vcfgv force-pushed the closure_clean_up branch 7 times, most recently from 92c68aa to d1ec187 Compare August 11, 2022 07:17
@vcfgv vcfgv force-pushed the closure_clean_up branch from d1ec187 to 3bcb798 Compare August 11, 2022 07:52
@vcfgv vcfgv marked this pull request as ready for review August 16, 2022 02:42
@vcfgv vcfgv requested a review from a team as a code owner August 16, 2022 02:42
@vcfgv vcfgv changed the title [WIP] Apply Operand Closure clean up Apply Operand Closure clean up Aug 16, 2022
chaokunyang
chaokunyang reviewed Aug 16, 2022
View reviewed changes
mars/dataframe/base/apply.py Outdated Show resolved Hide resolved
mars/dataframe/base/apply.py Outdated Show resolved Hide resolved
@chaokunyang
Copy link
Contributor

Can we add some tests to check that closure cleanup are actually executed?

@vcfgv vcfgv force-pushed the closure_clean_up branch from 9228143 to 960a7e4 Compare August 17, 2022 05:28
chaokunyang
chaokunyang previously approved these changes Aug 19, 2022
View reviewed changes
Copy link
Contributor

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

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

LGTM

fyrestone
fyrestone reviewed Aug 19, 2022
View reviewed changes
mars/dataframe/utils.py Outdated Show resolved Hide resolved
@vcfgv vcfgv force-pushed the closure_clean_up branch from 029e213 to 9ae0ea5 Compare August 19, 2022 08:50
zhongchun
zhongchun reviewed Aug 22, 2022
View reviewed changes
mars/dataframe/base/tests/test_base_execution.py Show resolved Hide resolved
mars/dataframe/base/tests/test_base_execution.py Show resolved Hide resolved
mars/dataframe/groupby/tests/test_groupby_execution.py Show resolved Hide resolved
mars/dataframe/utils.py Outdated Show resolved Hide resolved
mars/dataframe/utils.py Outdated Show resolved Hide resolved
mars/dataframe/utils.py Outdated Show resolved Hide resolved
mars/dataframe/utils.py Outdated Show resolved Hide resolved
mars/dataframe/utils.py Outdated Show resolved Hide resolved
mars/deploy/oscar/tests/test_local.py Outdated Show resolved Hide resolved
mars/services/task/execution/ray/context.py Outdated Show resolved Hide resolved
@vcfgv vcfgv force-pushed the closure_clean_up branch from 201395f to c73744f Compare August 22, 2022 09:38
fyrestone
fyrestone reviewed Aug 23, 2022
View reviewed changes
mars/dataframe/utils.py Outdated Show resolved Hide resolved
@vcfgv vcfgv force-pushed the closure_clean_up branch from 4080e71 to ef47200 Compare August 23, 2022 05:25
zhongchun
zhongchun approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@zhongchun zhongchun left a comment

Choose a reason for hiding this comment

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

LGTM.

fyrestone
fyrestone approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@fyrestone fyrestone left a comment

Choose a reason for hiding this comment

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

LGTM

@vcfgv vcfgv mentioned this pull request Aug 23, 2022
Open
@fyrestone fyrestone merged commit 148bed8 into mars-project:master Aug 24, 2022
aresnow1 pushed a commit to aresnow1/mars that referenced this pull request Sep 7, 2022
@vcfgv @aresnow1
Apply Operand Closure clean up (mars-project#3205)
28cfb33 
* Initially implemented closure clean up

* Initially implemented support for ray task mode

* Add an option to selectively clean up or disable this feature

* code refactor

* Try to fix vineyard pickle error

* code refactor

* code refactor and avoid duplicated logic key generation

* code refactor

* add additional tests and code refactor

* code refactor

* refactor additional tests

* misc

* code refactor

* improve procedure of storage_put to work regardless of context instances

(cherry picked from commit 148bed8)
aresnow1 pushed a commit to aresnow1/mars that referenced this pull request Sep 7, 2022
@vcfgv @aresnow1
Apply Operand Closure clean up (mars-project#3205)
dd5c9ec 
* Initially implemented closure clean up

* Initially implemented support for ray task mode

* Add an option to selectively clean up or disable this feature

* code refactor

* Try to fix vineyard pickle error

* code refactor

* code refactor and avoid duplicated logic key generation

* code refactor

* add additional tests and code refactor

* code refactor

* refactor additional tests

* misc

* code refactor

* improve procedure of storage_put to work regardless of context instances

(cherry picked from commit 148bed8)
aresnow1 pushed a commit to aresnow1/mars that referenced this pull request Sep 7, 2022
@vcfgv @aresnow1
Apply Operand Closure clean up (mars-project#3205)
afadd52 
* Initially implemented closure clean up

* Initially implemented support for ray task mode

* Add an option to selectively clean up or disable this feature

* code refactor

* Try to fix vineyard pickle error

* code refactor

* code refactor and avoid duplicated logic key generation

* code refactor

* add additional tests and code refactor

* code refactor

* refactor additional tests

* misc

* code refactor

* improve procedure of storage_put to work regardless of context instances

(cherry picked from commit 148bed8)
@hekaisheng hekaisheng mentioned this pull request Sep 7, 2022
Merged
2 tasks
qianduoduo0904 pushed a commit to qianduoduo0904/mars that referenced this pull request Sep 22, 2022
@vcfgv @qianduoduo0904
Apply Operand Closure clean up (mars-project#3205)
077c435 
* Initially implemented closure clean up

* Initially implemented support for ray task mode

* Add an option to selectively clean up or disable this feature

* code refactor

* Try to fix vineyard pickle error

* code refactor

* code refactor and avoid duplicated logic key generation

* code refactor

* add additional tests and code refactor

* code refactor

* refactor additional tests

* misc

* code refactor

* improve procedure of storage_put to work regardless of context instances

(cherry picked from commit 148bed8)
UranusSeven pushed a commit to xorbitsai/mars that referenced this pull request Sep 22, 2022
@vcfgv @UranusSeven
Apply Operand Closure clean up (mars-project#3205)
200a919 
* Initially implemented closure clean up

* Initially implemented support for ray task mode

* Add an option to selectively clean up or disable this feature

* code refactor

* Try to fix vineyard pickle error

* code refactor

* code refactor and avoid duplicated logic key generation

* code refactor

* add additional tests and code refactor

* code refactor

* refactor additional tests

* misc

* code refactor

* improve procedure of storage_put to work regardless of context instances

(cherry picked from commit 148bed8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fyrestone fyrestone fyrestone approved these changes

@zhongchun zhongchun zhongchun approved these changes

@chaokunyang chaokunyang chaokunyang left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@vcfgv @fyrestone @chaokunyang @zhongchun