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-43982: Raise FatalAlgorithmError in sizeExtendedness plugin #271

Merged
merged 2 commits into from Apr 25, 2024

Conversation

arunkannawadi
Copy link
Member

No description provided.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-43982 branch 2 times, most recently from 59fadac to f5d2dc3 Compare April 19, 2024 21:56
Copy link
Member Author

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

I had originally intended to catch this in __init__ but we cannot guarantee that the required shape algorithms are registered before this. We should allow them to be registered in any order, since the plugins are ordered only at run time.

@TallJimbo
Copy link
Member

A measurement plugin's __init__ should be called after the __init__ of all of its dependencies, and if that's not happening it's a bug we can fix in the framework. What I consider "registration" (adding the config entries to the RegistryField) does indeed happen out of order, but it should all be done before the first plugin __init__ fires.

@arunkannawadi
Copy link
Member Author

arunkannawadi commented Apr 22, 2024

You're absolutely right. The plugins are initialized according to their execution order value. So we can definitely move up this check during initialization which is still at runtime, but not during config validation time (as John had originally intended, I assume). A plugin's config knows nothing about other plugins.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-43982 branch 2 times, most recently from 6c7019e to 4425b5c Compare April 22, 2024 17:28
Comment on lines 706 to 707
if not all(("slot_Shape" in schema.getAliasMap().keys(),
"slot_PsfShape" in schema.getAliasMap().keys(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have to get the alias map here, or can you just ask "slot_Shape" in schema? I just checked and that appears to do what you'd want.

Copy link
Member Author

Choose a reason for hiding this comment

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

You'd have had to do "slot_Shape_xx" in schema and not what you typed, right? I could go either way, but I inclined towards the current implementation since it felt more generalizable without putting in the extra _xx by hand.

@@ -695,6 +701,16 @@ def getExecutionOrder(cls):

def __init__(self, config, name, schema, metadata):
SingleFramePlugin.__init__(self, config, name, schema, metadata)

# Check that the required columns are already in the schema.
if not all(("slot_Shape" in schema.getAliasMap().keys(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of all, just and them. There's on two things, no need for all.

@parejkoj
Copy link
Contributor

Testing this against ap_verify, this doesn't actually solve the problem. It looks like I need to clear all the slots in CalibrateImageTask, as the schema there has the slots defined but they're not pointing to anything. For example 'slot_PsfShape'->'base_SdssShape_psf', but the latter doesn't exist (hence the error message in the ticket). I think you need to check both that the Shape slots have been defined, and that the thing they point to resolves. I'm not sure the best way to do that.

I'll separately fix CalibrateImage to remove all the nonexistent slots (probably om DM-43973).

@arunkannawadi
Copy link
Member Author

I can confirm the same. The previous version had slots pointing to something even though those plugins (base_SdssShape_psf in this case) were not being run at all. As we all learnt recently, thanks to John and Jim, it's just pointing to a string to substitute. I modified to look for the actual field in the schema the slots are pointing to, and ap_verify fails immediately as it should. The actual fix to avoid the failure is in DM-43973, so that blocks this now.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Thanks, I think this does take care of all the necessary cases.

@arunkannawadi arunkannawadi merged commit 6f33917 into main Apr 25, 2024
2 checks passed
@arunkannawadi arunkannawadi deleted the tickets/DM-43982 branch April 25, 2024 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants