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.

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

TaskVine executor: add parsl serialize module to default library context #3604

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

colinthomas-z80
Copy link
Contributor

Description

This improves task throughput for taskvine serverless by caching the serialize module and base function definition in the library

Type of change

Choose which options apply, and delete the ones which do not apply.

  • New feature

@benclifford
Copy link
Collaborator

This needs cctools 7.13.0 (because thats the first release that has hoisting_modules) but I noticed that Parsl CI is still on 7.8.0 so I think there isn't any test coverage for this change.

@benclifford
Copy link
Collaborator

I tried this out on my laptop with serverless mode turned on: a pytest run reduces from 116 seconds to 20 seconds. So it would be good for timing considerations to switch our CI taskvine test over to this mode, perhaps.

and

parsl-perf --time 20 --config parsl/tests/configs/taskvine_serverless.py

goes from
6 tasks/sec
to
140 tasks/sec

Importing parsl.serialize will (in usual Python) import parsl first which will in turn import a very large part of the parsl package tree - because parsl/__init__.py imports a lot for re-export - to get a "batteries included" style of import parsl.

So I propose that importing plain parsl rather than parsl.serialize should result in the same performance change - I tried that out on my laptop and the results appear to have about the same distribution (by eyeball).

I'm a little unclear what happens for user code that would benefit from this kind of pre-load for other modules.

Copy link
Collaborator

@benclifford benclifford left a comment

Choose a reason for hiding this comment

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

see other comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants