-
Notifications
You must be signed in to change notification settings - Fork 211
WIP: Optimize TaskVineExecutor #3724
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
Draft
tphung3
wants to merge
23
commits into
Parsl:master
Choose a base branch
from
tphung3:opt-vine-ex
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
21e0fad
first draft with double serialization
tphung3 0e22cbd
fix bug
tphung3 c763dde
deduplicate fn serialization
tphung3 1c1a080
finish serialization deduplication
tphung3 228bc9f
fix bug dedup double serial
tphung3 1bf960e
add option for non-tmp staging dir
tphung3 349f2d9
context feature added
tphung3 3e899fd
add _ to hidden variable
tphung3 0c1db8b
use 1 mapping only
tphung3 e880f37
check monitoring first
tphung3 0844a5d
fix lint issues
tphung3 96246e1
fix bug in mapping of function names in executor
tphung3 4436e7a
fix flake8
tphung3 d0d55d6
add annotation
tphung3 85b7c43
new way to detect monitoring code
tphung3 194632b
add run_parsl_function
tphung3 4c0d1cf
fixes to update head
tphung3 4bbf422
comment out debug code
tphung3 273ad01
use tmp dir defaults to False
tphung3 3fe0172
fix dir name
tphung3 c81b16e
make parents dir if needed
tphung3 16dc486
makedirs not mkdir
tphung3 562d2d3
remove context refs during args serialization
tphung3 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
Oops, something went wrong.
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 a bit suspicious to me -- what is it about monitoring that is special here?
is it that there is a new function definition for every submission? If so, there are other parsl modes like some file staging scenarios, where that can happen too.
Although I think that is also what the block below, with function equality checking and fallback to regular is trying to do too?
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.
You are right that for serverless execution model to work, a function
funcmust be a generic function, instead of an invocation-specific function. The monitoring code replaces the original function with an invocation-specific version withtask_idand so on. For file staging, I have been poking around it and find that as long as thestorage_accessattribute is None, which it is forTaskVineExecutor, the dfk won't replace the original function with file staging wrapper (I might be wrong here) and file staging is instead delegated to TaskVine's specialized file staging API.The code block with function id checking is as you said a detection mechanism to fall back to the regular execution method if functions do change.
Thanks for reviewing it early! I was gonna fill this PR with more descriptions, tests, and documentation before pinging you.
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.
people don't use file staging much, but it might not be none - for example, I know one user who uses work queue plus zip file staging in Parsl, and that should translate directly to tv + zip file staging.
does that monitoring test detect anything that the line below it doesn't already detect? or is it for getting a nicer error message?
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.
It is for getting a nicer message so users may know what's wrong specifically with each case. The monitoring detection can be removed without affecting functionality.
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.
Actually the first monitoring check removes the case where the first function runs serverless-ly and subsequent functions run regularly.