HEPCloud / decisionengine

HEPCloud Decision Engine framework
Apache License 2.0
6 stars 26 forks source link

Support sources shared between channels #568

Closed knoepfel closed 2 years ago

knoepfel commented 2 years ago

This PR supports the sharing of a source between channels. It is the final PR required in removing the source-proxy system. A source is shared between channels if:

  1. Two or more channel configurations specify the same source name, and
  2. The configurations for that source name are identical in the the channel configurations.
pep8speaks commented 2 years ago

Hello @knoepfel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-01-18 17:43:25 UTC
codecov[bot] commented 2 years ago

Codecov Report

Merging #568 (3d2f95b) into master (afcc7cf) will increase coverage by 0.24%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
+ Coverage   95.97%   96.21%   +0.24%     
==========================================
  Files          46       47       +1     
  Lines        2759     2829      +70     
  Branches      439      447       +8     
==========================================
+ Hits         2648     2722      +74     
  Misses         77       77              
+ Partials       34       30       -4     
Flag Coverage Δ
python-3.10 95.93% <99.18%> (+0.06%) :arrow_up:
python-3.6 95.80% <99.45%> (+0.14%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/decisionengine/framework/modules/Source.py 100.00% <ø> (ø)
.../decisionengine/framework/engine/ChannelWorkers.py 88.15% <100.00%> (ø)
.../decisionengine/framework/engine/DecisionEngine.py 89.02% <100.00%> (-1.00%) :arrow_down:
...c/decisionengine/framework/engine/SourceWorkers.py 100.00% <100.00%> (ø)
...sionengine/framework/taskmanager/LatestMessages.py 100.00% <100.00%> (ø)
...engine/framework/taskmanager/SourceProductCache.py 100.00% <100.00%> (ø)
...ecisionengine/framework/taskmanager/TaskManager.py 97.43% <100.00%> (-0.84%) :arrow_down:
...cisionengine/framework/taskmanager/module_graph.py 100.00% <100.00%> (ø)
src/decisionengine/framework/modules/Module.py 94.59% <0.00%> (-5.41%) :arrow_down:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update afcc7cf...3d2f95b. Read the comment docs.

mambelli commented 2 years ago

A couple of comments pre-review:

Then would be great if you can add some more tests especially to increase coverage of the source worker and its use in DecisionEngine.py. Thanks

knoepfel commented 2 years ago

@mambelli:

in the ChannelWroker you removed the logger for the process (self.logger, was line 59). Was that on purpose because there are no messages?

That's correct. I restored the self.logger attribute in the latest push, but I'm still not convinced it's needed.

framework/taskmanager/source_queues.py is an empty file. Is it a placeholder for future code?

Oops--that file should have been removed. See latest push.

Then would be great if you can add some more tests especially to increase coverage of the source worker and its use in DecisionEngine.py.

This is a source of frustration for me--I believe that in order for the unit tests to pass, almost all aspects of the source workers must be used. I'm guessing there's a code-coverage issue. I'll investigate a little further.

jcpunk commented 2 years ago

The recording of coverage for multiprocess workers is a bit spotty at times...

knoepfel commented 2 years ago

Reopening in attempt to fix code-coverage issues.

jcpunk commented 2 years ago

pre-commit.ci run

jcpunk commented 2 years ago

This looks pretty good to me. Can we merge the "Another Try" commit into one of the others?

knoepfel commented 2 years ago

@jcpunk, yes I will merge "Another try" with one of the others. There are some docstrings and a couple more unit tests I should add. I hope to have this PR polished up by tomorrow or Friday...assuming, of course, that the tests all pass.

knoepfel commented 2 years ago

I believe this PR is complete--I do not anticipate any more changes except that I will rebase off of master once #598 is merged. Please feel free to review.