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

Add concurrent.futures.Executor implementation #235

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

nchristensen
Copy link
Contributor

@nchristensen nchristensen commented Nov 21, 2022

This adds a concurrent.futures.Executor implementation for Charm4py so Charm4py can be dropped in wherever ThreadPoolExecutor, ProcessPoolExecutor, MPI4pyPoolExecutor, etc. is used.

A PoolExecutor class in pool.py implements the Executor API by calling to equivalent Pool methods. This pull request also makes charm4py.threads.Future inherit from concurrent.futures.Future and implements its API.

TODO:

  • Fix Future.set_running_or_notify_cancel() and figure out where to call it. Execution currently hangs without explicitly calling Future.get().
  • Update dependencies
  • Update documentation
  • Add tests
  • Increment Charm4py version
  • Add _max_workers property to Executor

@lgtm-com
Copy link

lgtm-com bot commented Nov 21, 2022

This pull request introduces 2 alerts when merging 1b08833 into 74791f9 - view on LGTM.com

new alerts:

  • 1 for Signature mismatch in overriding method
  • 1 for First parameter of a method is not named 'self'

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Nov 21, 2022

This pull request introduces 2 alerts when merging 08cf569 into 74791f9 - view on LGTM.com

new alerts:

  • 1 for Signature mismatch in overriding method
  • 1 for First parameter of a method is not named 'self'

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@ZwFink
Copy link
Contributor

ZwFink commented Nov 21, 2022

Hi Nick,
This is some really cool work! I can help out later this week or early next week to help with testing and documentation.

@nchristensen
Copy link
Contributor Author

Cool, that would be great! It could definitely use another pair of eyes.

@lgtm-com
Copy link

lgtm-com bot commented Nov 22, 2022

This pull request fixes 1 alert when merging 9822860 into 74791f9 - view on LGTM.com

fixed alerts:

  • 1 for Illegal raise

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Dec 6, 2022

This pull request fixes 1 alert when merging 282a11b into 74791f9 - view on LGTM.com

fixed alerts:

  • 1 for Illegal raise

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Dec 6, 2022

This pull request fixes 1 alert when merging b71e4b3 into 74791f9 - view on LGTM.com

fixed alerts:

  • 1 for Illegal raise

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Dec 7, 2022

This pull request introduces 7 alerts and fixes 2 when merging 081d3dc into 74791f9 - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 2 for Unreachable code

fixed alerts:

  • 1 for Missing call to `__init__` during object initialization
  • 1 for Illegal raise

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Dec 7, 2022

This pull request introduces 7 alerts and fixes 2 when merging 2bbb9a8 into 74791f9 - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 2 for Unreachable code

fixed alerts:

  • 1 for Missing call to `__init__` during object initialization
  • 1 for Illegal raise

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@nchristensen nchristensen marked this pull request as draft December 8, 2022 00:40
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.

None yet

2 participants