Conversation
There was a problem hiding this comment.
Returning this review first since I'm asking for a lot of changes. I don't think it makes sense to split the service configuration across two variables, or to have it in both variables and config. I'd also like to take another look at the networking code once it's been cleaned up.
Also, please clean up the commit history, merging all the "fixup" commits and editing the commit message. Lines like "Fix" and "More Fix" aren't really helpful to someone looking at the repo history.
(I also agree with everything K-T said.)
| sattle_output = requests.put( | ||
| f'{self.config.sattle_host}:{self.config.sattle_port}/diasource_allow_list', | ||
| json={"visit_id": visit_id, "detector_id": detector_id, "diasources": dia_sources_json}) |
There was a problem hiding this comment.
Outside the scope of this review, but PUT doesn't seem like the right method for a query. Should this be GET/POST instead?
|
@kfindeisen Here's a potential restructuring of the code--if you're happy with the general direction I'll rework the tests. Ditto in pipe_tasks. Not linted, obviously. |
kfindeisen
left a comment
kfindeisen
left a comment
There was a problem hiding this comment.
Looks much better, main objection is that prepareSattleDiaSourcePayload does half the work of querying in a way that forces its caller to still have to deal with all the network stuff. Other comments are details.
ebellm
force-pushed
the
tickets/DM-50987
branch
2 times, most recently
from
407a7e8 to
55db76b
Compare
kfindeisen
left a comment
kfindeisen
left a comment
There was a problem hiding this comment.
Looks good, but please do not use "respond to review" commits. They're specifically disallowed by policy, and they do nothing but make it harder for anyone to tell what's actually changed.
| with self.assertRaises(requests.exceptions.HTTPError): | ||
| detectionTask.run(science, matchedTemplate, difference, sources, | ||
| idFactory=IdFactory.makeSimple()) |
There was a problem hiding this comment.
Does this conform to the rules for how tasks report errors (which I'll admit I'm not up to date on)?
Add the sattle filter call to detectAndMeasure, sending the bboxes and source id's to the sattle api. It then returns a list of acceptable sources, which is used to remake the diaSource catalog containing only the required sources.
4 participants
No description provided.