Fix two bugs and add support for N masters, N workers with load balancing in RadicalPilotExecutor #3060
Conversation
addressing comments and refine
benclifford
changed the title
| """ | ||
| for fut_task in self.future_tasks.values(): | ||
| if not fut_task.done(): | ||
| fut_task.set_exception(exception) |
There was a problem hiding this comment.
maybe there's a very slight race condition here - can fut_task get set by another thread (eg. task completion) in the space between line 211 and line 212? If so, set_exception would raise an exception here. I've seen a few situations like that elsewhere in Parsl.
There was a problem hiding this comment.
I do not think so. State_Callbacks is the only one responsible for setting fut_tasks in the executor.
|
ok thanks. there are a bunch of changes in this and I'm not super familiar with this bit of the code, so I'll try to use the review as a learning experience. If you're making several changes at once, it can be easier both for review and tracking down later bugs to make several smaller, single focused PRs (a rule of thumb for me is, if you start listing out multiple changes in the PR description then consider splitting it...) |
@benclifford totally understandable and I agree. In the future, the PRs will be more focused. During this review please let me know if I can help you with anything vague or any questions. Thank you and sorry for the late response.. |
| @@ -10,6 +10,7 @@ export PATH := $(CCTOOLS_INSTALL)/bin/:$(PATH) | |||
| export CCTOOLS_VERSION=7.7.2 | |||
| export HYDRA_LAUNCHER=fork | |||
| export OMPI_MCA_rmaps_base_oversubscribe=yes | |||
| export RADICAL_TEMP_SANDBOX=True | |||
There was a problem hiding this comment.
i think this variable is not interpreted as a Python bool so it's maybe misleading to use True here in case someone thinks False would do something opposite.
There was a problem hiding this comment.
I agree with that. maybe RADICAL_TEMP_SANDBOX=yes or RADICAL_TEMP_SANDBOX=1 ?
| # to include the agent sandbox with the ci artifacts. | ||
| if os.environ.get("LOCAL_SANDBOX"): | ||
| pd_init['sandbox'] = self.run_dir | ||
| # set the agent dir to /temp as a fix for #3029 |
There was a problem hiding this comment.
Is this changed sandbox behaviour the only part that fixes #3029?
If so:
it's moving things into a temporary directory only when the CI tests are run (or in other situations where the RADICAL_TEMP_SANDBOX environment variable is set).
in non-test situations, this behaviour is otherwise unchanged, right? If so, does this PR stop that situation occurring for normal users who are running real applications, not our CI?
There was a problem hiding this comment.
While this is a very rare case that can happen when the filesystem is exhausted for some reason (maybe too many concurrent I/O or so in the CI test machine), this behavior should not happen on a regular basis.
I do understand your concerns here that this might also happen specifically on a local non-test setup (as the agent sandbox is located in $HOME), we never observed this behavior on an HPC machine as we use the shared file system to store our agent sandbox.
I think to address your concern we can make the default location for the sandbox on local setup in general inside the /tmp folder? I am open to suggestions as well.
There was a problem hiding this comment.
I investigated this a bunch more (and have commented on what I see in other issues) to try to understand what was actually happening in the CI environment, because I like to at least have some understanding of how that environment behaves.
I can see why this is happening in the CI (due to entering a strange zone of fd number-space which radical.saga cannot deal with) and therefore why this would happen in a user workflow (when a user's workflow submitting process has opened many - around 1000 - files).
This PR avoid the misbehaving radical.saga code path by having a special non-default option to turn on a local sandbox.
But in radical-cybertools/radical.saga#885 Andrew mentions, effectively, that radical.saga would be better replaced by psij-python - that would have the same effect of removing the misbehaving codepath, in a different way.
So I tried that out in this draft PR: #3079 which doesn't actually avoid the misbehaving codepath, as far as I can tell.
But maybe this is a better approach longer term, if radical.saga is effectively deprecated in favour of psij-python?
Description
This PR:
Fixes
Fixes #3029, #3070
Type of change
Choose which options apply, and delete the ones which do not apply.