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-43472: Prompt Processing version 2 not compatible with Butler dimensions-config 6 #144

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

kfindeisen
Copy link
Member

@kfindeisen kfindeisen commented Mar 23, 2024

This PR adds a day_obs dimension to the fix merged on #143; for some reason the tests with that PR passed without it.

Early tests suggested that day_obs was not needed, but this seems to not be the case.
@kfindeisen
Copy link
Member Author

kfindeisen commented Mar 23, 2024

I suspect the day_obs bug only appears once per day and that the table rows can come from a source other than PP, so I will confirm tomorrow morning before merging.

@kfindeisen kfindeisen merged commit 5f1c220 into main Mar 24, 2024
5 checks passed
@kfindeisen kfindeisen deleted the tickets/DM-43472-hotfix branch March 24, 2024 16:00
@timj
Copy link
Member

timj commented Mar 25, 2024

It should be possible to do a more general fix that automatically finds dependent records and includes them so you would only need to list the dimensions you really want and then other dimensions would be found. We sort of do something similar in the Butler.transfer_dimension_records_from internals.

@kfindeisen
Copy link
Member Author

kfindeisen commented Mar 25, 2024

I thought that was what populated_by was for? But neither group nor day_obs use it.

(And we specifically don't want to push all dependent records, because of deadlock problems; otherwise we could just use transfer_from(transfer_dimensions=True).)

@timj
Copy link
Member

timj commented Mar 25, 2024

day_obs is not populated by anyone as such (whereas visit_detector_region is) but you can ask exposure which dimensions it uses itself and so if you have exposure in the list you can automatically know that group/day_obs/physical_filter/instrument will be needed. There is also a sorting API that lets you know which order you need to insert dimension records such that the dependencies are met.

@kfindeisen
Copy link
Member Author

Right, instrument was one of the ones that deadlocked before we implemented it this way.

@timj
Copy link
Member

timj commented Mar 25, 2024

Yes, but you can add an exclusion list for physical_filter/instrument/detector that have to be present via instrument registration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants