HEPCloud / decisionengine

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

Introduce publisher status data product #668

Closed knoepfel closed 1 year ago

knoepfel commented 2 years ago

As part of the channel redesign effort, this PR:

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-09-07 19:49:37 UTC
codecov[bot] commented 2 years ago

Codecov Report

Base: 97.27% // Head: 97.31% // Increases project coverage by +0.03% :tada:

Coverage data is based on head (34cfef1) compared to base (e0749d8). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #668 +/- ## ========================================== + Coverage 97.27% 97.31% +0.03% ========================================== Files 50 51 +1 Lines 3123 3162 +39 Branches 561 567 +6 ========================================== + Hits 3038 3077 +39 Misses 54 54 Partials 31 31 ``` | Flag | Coverage Δ | | |---|---|---| | python-3.10 | `97.24% <100.00%> (+0.03%)` | :arrow_up: | | python-3.6 | `97.04% <100.00%> (+0.06%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/HEPCloud/decisionengine/pull/668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud) | Coverage Δ | | |---|---|---| | [.../decisionengine/framework/engine/DecisionEngine.py](https://codecov.io/gh/HEPCloud/decisionengine/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud#diff-c3JjL2RlY2lzaW9uZW5naW5lL2ZyYW1ld29yay9lbmdpbmUvRGVjaXNpb25FbmdpbmUucHk=) | `93.63% <100.00%> (+<0.01%)` | :arrow_up: | | [...ionengine/framework/taskmanager/PublisherStatus.py](https://codecov.io/gh/HEPCloud/decisionengine/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud#diff-c3JjL2RlY2lzaW9uZW5naW5lL2ZyYW1ld29yay90YXNrbWFuYWdlci9QdWJsaXNoZXJTdGF0dXMucHk=) | `100.00% <100.00%> (ø)` | | | [...ecisionengine/framework/taskmanager/TaskManager.py](https://codecov.io/gh/HEPCloud/decisionengine/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud#diff-c3JjL2RlY2lzaW9uZW5naW5lL2ZyYW1ld29yay90YXNrbWFuYWdlci9UYXNrTWFuYWdlci5weQ==) | `97.51% <100.00%> (+0.06%)` | :arrow_up: | | [...cisionengine/framework/taskmanager/module\_graph.py](https://codecov.io/gh/HEPCloud/decisionengine/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud#diff-c3JjL2RlY2lzaW9uZW5naW5lL2ZyYW1ld29yay90YXNrbWFuYWdlci9tb2R1bGVfZ3JhcGgucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPCloud)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

StevenCTimm commented 2 years ago

For what it's worth--yes the publisher should be able to query the status of a publisher..although there's no known use case for it, it is best to have data blocks universally readable--if you don't want a given publisher to read it then just don't have it CONSUME the block.

As far as re-enabling a publisher that has been disabled, as mentioned before IMO this should be the province of the logic engine. Could easily add the publisher status into existing logic engine expressions and this should do the trick.

StevenCTimm commented 2 years ago

Also as discussed in the Sep. 8 decision engine meeting, the PublisherStatus should reflect only the final status after all the retries in the case where the publisher is configured in the framework config to automatically make N number of retries, which all publishers in production are.