pytroll / pytroll-pps-runner

Pytroll runner for PPS
GNU General Public License v3.0
1 stars 8 forks source link

Fix consistent publish topic or specify in yaml file #25

Closed adybbroe closed 3 years ago

adybbroe commented 3 years ago

Change the mandatory fields required in the yaml file for the pps-hook. Removing station and publish_topic as mandatory parameters and adding three more. Now the publish topic is either taken from here (if posttroll_topic is specified) or being generated from these required parameters in a hopefully consistent way. The list of mandatory parameters are:

{'level': 'data_processing_level',
  'output_format': 'format',
  'variant': 'variant',
  'geo_or_polar': 'geo_or_polar',
  'software': 'software'

This applies if a posttroll_topic is not provided. Then the above parameters are used to create a consistent posttroll publish topic. If such a posttroll_topic is available, the other fields are not required at the moment at least.

So, a pps_hook.yaml file may look like this, and be okay:

pps_hook:
    post_hook: !!python/object:nwcsafpps_runner.pps_posttroll_hook.PPSMessage
      description: "This is a pps post hook for PostTroll messaging"
      metadata:
        publish_topic: "/my/pps/publish/topic/"

In this case the messages published from PPS will be like this: pytroll: //my/pps/publish/topic/CTTH for the CTTH product, and pytroll: //my/pps/publish/topic/CMAProb for the cloud probability product.

Or it can be as simple as this:

pps_hook:
    post_hook: !!python/object:nwcsafpps_runner.pps_posttroll_hook.PPSMessage
      description: "This is a pps post hook for PostTroll messaging"
      metadata:
        station: "mystation"
        output_format: "CF"
        level: "2"
        variant: DR
        geo_or_polar: "polar"
        software: "NWCSAF-PPSv2018"
codecov[bot] commented 3 years ago

Codecov Report

Merging #25 (41d3ea4) into master (2526dd0) will increase coverage by 1.58%. The diff coverage is 61.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   29.82%   31.40%   +1.58%     
==========================================
  Files          12       12              
  Lines        1747     1761      +14     
==========================================
+ Hits          521      553      +32     
+ Misses       1226     1208      -18     
Flag Coverage Δ
unittests 31.40% <61.46%> (+1.58%) :arrow_up:

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

Impacted Files Coverage Δ
nwcsafpps_runner/metno_update_nwp.py 0.00% <0.00%> (ø)
nwcsafpps_runner/pps2018_runner.py 0.00% <0.00%> (ø)
nwcsafpps_runner/pps_runner.py 0.00% <0.00%> (ø)
nwcsafpps_runner/prepare_nwp.py 0.00% <0.00%> (ø)
nwcsafpps_runner/utils.py 26.84% <50.00%> (ø)
nwcsafpps_runner/pps_posttroll_hook.py 79.64% <100.00%> (+0.49%) :arrow_up:
nwcsafpps_runner/tests/test_pps_hook.py 100.00% <100.00%> (ø)

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 2526dd0...41d3ea4. Read the comment docs.

ghost commented 3 years ago

Congratulations :tada:. DeepCode analyzed your code in 0.627 seconds and we found no issues. Enjoy a moment of no bugs :sunny:.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

adybbroe commented 3 years ago

Fixed major part of the flake8 issues. There are still a handful left. But I leave these for another PR.

adybbroe commented 3 years ago

@TAlonglong I was quite tough on your nwp code, and removed unused code parts. So, not sure this still works for you. But, as you said perhaps you can fix it when you are ready to upgrade!?

adybbroe commented 3 years ago

Anyone cares if I merge this one? @mraspaud @pnuu or @TAlonglong ?

pnuu commented 3 years ago

I don't handle PPS personally, so don't have any experience with the runner either. But I hope it's still possible to define the publish_topic by the user.

There seems to be quite a lot of changes that don't match the title of the PR, which makes the PR a bit confusing to go through.

adybbroe commented 3 years ago

I don't handle PPS personally, so don't have any experience with the runner either. But I hope it's still possible to define the publish_topic by the user.

There seems to be quite a lot of changes that don't match the title of the PR, which makes the PR a bit confusing to go through.

Sorry. Changes here should mainly concern the hook that is used to have PPS sending potstroll messages once PPS finish. I am not sure the user have full power over what topic PPS publish, and I am not sure he/she should have!?

But I will give it some consideration

pnuu commented 3 years ago

I am not sure the user have full power over what topic PPS publish, and I am not sure he/she should have!?

But it isn't PPS that publishes the messages, but the runner. Personally, I'd want to use my own logic for naming the published topic so that it matches the logic in the other parts of the chain. This holds for every Pytroll package that either subscribes or publishes anything.

adybbroe commented 3 years ago

@pnuu I agree with your statement about being able to specify the publish topic as you wish yourself! I have therefore backed down on some of the changes and adapted accordingly. It maybe that the additional parameters are not needed. But I will leave it in for now, and hopefully closing this PR. I have renamed as well!