WIP: Optimize TaskVineExecutor #3724
Conversation
| # Fall back to regular execution if a function is Parsl-monitored as a monitored function is invocation-specific. | ||
| # Note that it is possible to get the wrapped function by calling the `__wrapped__` attribute when monitoring is enabled. | ||
| # It will disable the monitoring wrapper code however. | ||
| if exec_mode == 'serverless' and is_monitoring_enabled: |
There was a problem hiding this comment.
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.
You are right that for serverless execution model to work, a function func must be a generic function, instead of an invocation-specific function. The monitoring code replaces the original function with an invocation-specific version with task_id and so on. For file staging, I have been poking around it and find that as long as the storage_access attribute is None, which it is for TaskVineExecutor, 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.
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.
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.
Actually the first monitoring check removes the case where the first function runs serverless-ly and subsequent functions run regularly.
Description
Please include a summary of the change and (optionally) which issue is fixed. Please also include
relevant motivation and context.
Changed Behaviour
Which existing user workflows or functionality will behave differently after this PR?
Fixes
Fixes # (issue)
Type of change
Choose which options apply, and delete the ones which do not apply.