Skip to content
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

DM-34964: add cancel, restart and ping function in the bps panda plugin #11

Merged
merged 18 commits into from Jul 7, 2022

Conversation

wguanicedew
Copy link
Contributor

@wguanicedew wguanicedew commented Jun 6, 2022

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming @MichelleGower will also take a look from a bps perspective.

I have many comments but I think most of them are easy to deal with.

I do have a concern about the return values from idds APIs being very hard to use. It looks like there are tuples with tuples and the code here would benefit significantly from those APIs returning objects that can be inspected with methods.

Code like this:

if ret[0] == 0 and ret[1][0]

is very hard to follow and would be much easier if ret was an object that could have methods run on it.

doc/changes/DM-34964.rst Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay now. Thanks for the changes. Some minor comments and I'll leave the rest for @MichelleGower.

Please consider using pre-commit so that the code is black-compatible before it is committed.

python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MichelleGower MichelleGower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my tests, report isn't working as expected. Also, the behavior in error cases isn't the same as the expected behavior for all of the new functions. And a heads up that the expected ping method signature will be different than it appears here.

python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
python/lsst/ctrl/bps/panda/panda_service.py Outdated Show resolved Hide resolved
@wguanicedew wguanicedew changed the title DM-34964: add cancel, restart, ping and report function in the bps panda plugin DM-34964: add cancel, restart and ping function in the bps panda plugin Jun 24, 2022
@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #11 (be97783) into main (0005c0f) will decrease coverage by 2.62%.
The diff coverage is 10.66%.

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   35.28%   32.65%   -2.63%     
==========================================
  Files           9        9              
  Lines         479      539      +60     
  Branches       79       91      +12     
==========================================
+ Hits          169      176       +7     
- Misses        307      360      +53     
  Partials        3        3              
Impacted Files Coverage Δ
python/lsst/ctrl/bps/panda/idds_tasks.py 31.62% <0.00%> (ø)
python/lsst/ctrl/bps/panda/panda_service.py 20.45% <10.95%> (-4.55%) ⬇️

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 0005c0f...be97783. Read the comment docs.

@wguanicedew wguanicedew merged commit 16c11f7 into main Jul 7, 2022
@wguanicedew wguanicedew deleted the tickets/DM-34964 branch July 7, 2022 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants