-
Notifications
You must be signed in to change notification settings - Fork 39
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
remove some deepcopy to speed up workflow conductor #256
base: master
Are you sure you want to change the base?
Conversation
guzzijones
commented
Jul 7, 2023
•
edited
Loading
edited
- removing deepcopy where we can
- change deepcopy where still needed to custom copy command to increase speed using instead a shallow copy
- add some data classes * tsc meeting comment
remove 3.6 tox for now
The use of 'latest' is causing my tox unit testing to fail it appears.
I also had to disable python 3.6 for now. |
set ubuntu-20.04
stevedore is not working in the python 3,8 unit tests
@m4dcoder can you please take a look at this python 3.8 test failing. It looks like stevedore is not able to find the native, mock plugins to run the tests in python 3.8. |
I did some testing with st2 and this definitely breaks with-items no longer report failure to the parent workflow.
|
We won't be able to add a feature flag without a change in ST2. Orquesta does not have the Stackstorm config object afaik. |
Thanks.
Yeah, I assume it would probably also require a change on the StackStorm side. If you have cycles, I still think that would be a good idea. Still much easier to flip the feature flag / config option in case something goes wrong versus needing to downgrade to a previous release. Especially if it's combined with a breaking backward incompatible change in StackStorm core (live action FK change - there is only a forward migration in that PR, but not a backward one). |
I've been looking into some slowness for workflows with a large amount of items inside of a with items step, I saw this PR and I'm curious what items are we actually looking to have gated by a feature flag in this PR? |
task render speed gain when removing deep copy for large parameters |
I added a micro benchmark to show how much faster it is to NOT do a deep copy. I could add it for the serialize and deserialize, but the results will be the same. Copying large objects is costly. |
all the 3.9 tests passed with these updates minus the lint checks). |
I am running this additional speed up on our internal fork of St2. It looks like ci/cd failed because black changed with their latest release. |
@Kami @guzzijones are we requiring the feature flag to switch between deepcopy and fastdeepcopy for this to move forward? |
Note, this will help with "with items" speed issues. |
@Kami is not active on this project anymore it seems
@nzlosh @cognifloyd please take a peak at this. I adds significant speed improvements by eliminating some deep copies. This helps with larger context objects. |
Anyone willing to give some feedback here? |
Technically I'm OK with this to be merged. As discussed mid 2024, it should be merged as part of st2 3.10. I think we should leave a release between the pymongo updates and the deepcopy change to avoid confusing any issues that may come up in the 3.9 and 3.10 release. |