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

Update auto follow-up request priority #252

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Oct 4, 2023

This PR does:

  • when looking for existing follow-up requests, also look if the existing requests are from the same group, and have the same payload.
  • if a request exists, but the last candidate passed a filter that yielded a higher priority than the existing request's priority, update it with the new priority value.

Also, a minor change to try to get around the current issues with missing data: When building the "all_prv_candidates"/alert_history list used for ML feature generation, also append the full history of alerts, not just prv_candidates. This should help BTSbot not have high scores on old variables.

A few questions for the reviewers:
@mcoughlin is it correct that updating a request's priority (so effectively, the payload) tells the instrument of the allocation to update it? Just making sure it doesn't just give us the illusion to do so. If that's the case, we might want to delete and recreate instead, which comes with it's own challenges.
-> UPDATE: I checked and it should! At least this is implemented for SEDM's API class.

@nabeelre, given the additional cut on group_id when looking for existing requests, this means that if SEDM has pending or completed requests with either another allocation, or the same allocation but not shared with the auto-triggering group where you have your filter (which should be the case for existing requests), or has a different payload, ... and hasn't been classified and didn't get a spectra, we might still trigger SEDM. Just wanted to make sure that this is the expected behavior. I think it is, given the data access constraints we talked about earlier.

@Theodlz Theodlz requested a review from mcoughlin October 4, 2023 22:48
@mcoughlin
Copy link
Collaborator

@Theodlz updating should do what it says. If the schedulers don't do that (the just in time ones for sure do) then that is on the schedulers.

Copy link
Collaborator

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

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

LGTM

@nabeelre
Copy link

nabeelre commented Oct 4, 2023

Yup, that logic follows the intended behavior. Commit looks good to me!

@Theodlz Theodlz merged commit bd886c7 into skyportal:main Oct 5, 2023
2 checks passed
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.

3 participants