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 ConcurrentExecution step #511

Merged
merged 16 commits into from
Apr 21, 2024
Merged

Add ConcurrentExecution step #511

merged 16 commits into from
Apr 21, 2024

Conversation

gtopper
Copy link
Collaborator

@gtopper gtopper commented Apr 11, 2024

ML-6015

Based on the preexisting internal _ConcurrentJobExecution class. With support for three concurrency mechanisms: asyncio, threading, and multiprocessing.

Based on the preexisting internal `_ConcurrentJobExecution` class. With support for three concurrency mechanisms: asyncio, threading, and multiprocessing.
@assaf758 assaf758 requested a review from alxtkr77 April 17, 2024 05:50
storey/flow.py Outdated Show resolved Hide resolved
Copy link
Member

@alxtkr77 alxtkr77 left a comment

Choose a reason for hiding this comment

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

Aside from the issue with serializable context, everything else in the pull request looks good. Do we want to address this issue within the current pull request? It seems that the 'pass_context' flag may not be the best solution for the problem.

@gtopper
Copy link
Collaborator Author

gtopper commented Apr 17, 2024

Do we want to address this issue within the current pull request?

To make mlrun context serializable isn't something we can do in storey anyway.

It seems that the 'pass_context' flag may not be the best solution for the problem

Why is it the flag's fault? :) Do you have some change in mind?

@gtopper gtopper requested a review from alxtkr77 April 17, 2024 10:28
@gtopper gtopper closed this Apr 17, 2024
@gtopper gtopper reopened this Apr 17, 2024
@assaf758
Copy link
Member

Do we want to address this issue within the current pull request?

To make mlrun context serializable isn't something we can do in storey anyway.

It seems that the 'pass_context' flag may not be the best solution for the problem

Why is it the flag's fault? :) Do you have some change in mind?

Can you check if indeed https://github.com/uqfoundation/dill can serialize lambdas?

@gtopper
Copy link
Collaborator Author

gtopper commented Apr 18, 2024

@assaf758, it can, but it's not obvious how to use that to solve the problem. I'll try to find a way.

@gtopper
Copy link
Collaborator Author

gtopper commented Apr 18, 2024

It turns out that mlrun GraphContext contains the entire graph, which even dill fails to serialize. Ironically, it's there for ParallelRun.

@gtopper gtopper merged commit bbc644f into mlrun:development Apr 21, 2024
3 checks passed
gtopper added a commit to gtopper/storey that referenced this pull request Apr 21, 2024
gtopper added a commit that referenced this pull request Apr 21, 2024
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.

3 participants