Enhancement #136 - Dynamic flow creation for workers #137
Conversation
There was a problem hiding this comment.
I really like this, thanks! What I would probably change to make it more generic:
Currently the worker is statefull - it keeps configuration of the last run with specified configuration. So if I start selinon with some configuration, it gets overwritten with a run where configuration is passed in run_flow.
From user's perspective, I would maybe expect that if I do not pass configuration to run_flow, there is used the one that was used during start up. If there was passed the config option, it is used only for the requested flow run (and its nested flows).
What do you think?
| @@ -162,6 +164,7 @@ def __init__(self, dispatcher_id, flow_name, node_args=None, retry=None, state=N | |||
| # Instantiate lazily later if we will know that there is something to process | |||
| self._waiting_edges = [] | |||
| self._retry = retry | |||
| self._config = copy.copy(config) | |||
There was a problem hiding this comment.
During parallel execution a different reference of config was needed hence use the copy.
There was a problem hiding this comment.
Was it process level parallel execution?
There was a problem hiding this comment.
yes, it for each individual process
selinon/run.py
Outdated
| config_module['dispatcher_queues'] = Config.dispatcher_queues | ||
| config_module['task_queues'] = Config.task_queues | ||
| config_module['nodes_definition'] = nodes_definition | ||
| config_module['flow_definitions'] = flow_definitions |
There was a problem hiding this comment.
This could be accomplished by setting the config using set_config_dict, but see my review comments above.
There was a problem hiding this comment.
As the request is dispatched from the run, the configuration would not be available at celery execution. Hence tried updating the Config class, which will make use of the node, flow definition and reconstruct the Config.
There was a problem hiding this comment.
Agree, these values should be sufficient to kick-off flow run from message producer. My comment was more on reusing the logic from set_config_dict, but I see your point.
There was a problem hiding this comment.
Hi Fridex,
Thank you, yes set_config_dict method can be reused and i have modified the PR. Please review
|
Hi fridex, Ya i agree that under run_flow if no configuration is provided then flow should consider the default configuration during startup. I would change the run_flow method. Thanks, Harish |
… is not passed for run_flow method
|
Hi Fridex, Any suggestion on this pull request that needs to be modified or we are good to go with merge. I tried executing test case as well but i am getting few errors. Please suggest how to proceed. Thanks & Regards |
|
There seem to be linting issues - see reports in Travis CI - for example - https://travis-ci.org/selinon/selinon/jobs/461193958. Could you take a look at them? I would like to have some tests for this so we make sure this feature does not break with any of the future releases and add this feature to docs - I can make this happen in my spare time. Please make sure there is checked "Allow edits from maintainers." on the right panel. Thanks again for your contribution! |
|
Also, I think the worker looks statefull with this change - see my comments above. |
|
Hi Fridex, There are no pylint errors from the files which i have modified have attached the screen short for reference Thanks & Regards, |
I set up Travis CI to make these things reproducible - see the Travis CI reports here - https://travisci.org/selinon/selinon/pull_requests in pull request 137 (or ckick on "Details" in the checks window below this comment in failed checks). I use some custom configurations of pylint.
Thanks, it looks good to me. Thanks for your time! |
|
Hi Fridex, Sorry i am not able to get the custom configuration w.r.t pylint and the link provided above is not working. Please let me know the custom configuration changes so that i can fix those pylilnt issues. Thanks & Regards |
|
Oh, sorry. The link was incorrectly copied - https://travis-ci.org/selinon/selinon/pull_requests |
|
Hi Fridex, I was not able to find the pylint issues and i tried configuring in my local as well but those warning what i am getting is not because of the changes that i have made in my PR. could you please help me in fixing pylint issues. I have attached the screen shot of pylint running in my local. |
|
You can try to run |
|
Master should be fixed in #138. You can rebase to see if issues persist. |
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
==========================================
- Coverage 68.25% 68.18% -0.07%
==========================================
Files 62 62
Lines 4136 4149 +13
==========================================
+ Hits 2823 2829 +6
- Misses 1313 1320 +7
Continue to review full report at Codecov.
|
|
Hi Fridex, Thank you very much for the help pylint issues have been fixed and build has passes but it has failed with the test coverage. Please let me know what to do on those issues. Thanks & Regards |
|
Hi Fridex, May i know the status of the above Pull Request. Thanks & Regards |
|
Oh, I'm really sorry - somehow I missed notification around this one. The only concern I have is the behavior when a worker is started with some configuration which gets overwritten with this approach. |
If this pull request fixes a bug or resolves a feature, please link to that issue (e.g. fixes: #1).