-
Notifications
You must be signed in to change notification settings - Fork 0
Improved Alert Handling #688
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was
linked to
issues
Feb 18, 2025
Closed
kzscisoft
approved these changes
Feb 19, 2025
kzscisoft
requested changes
Feb 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please solve conflict
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improved Alert Handling
Improves the alert deduplication - a new query parameter 'deduplicate' can be provided to the
post
request, which will return a 409 CONFLICT if that alert definition already exists on the server.Made it so that this is the default behaviour, assigning a
self._params = {'deduplicate': True} in the
newmethod of each alert type. Could not add it to the
initof the alert as this was then overriden by the
self_params = {}` in the BaseObject init.Also added a new
attach_to_run
parameter to allcreate_xyz_alert
methods in theRun
class - if disabled, the alert will be created on the server but not linked to any run. Because of this, I also removed thecheck_run_initialized
decorator from these methods, and moved it to theattach_to_run
method. The benefit of this is you can now start a run indisabled
mode to define alerts on the server without creating a whole new run to do so. This is used in the integrations repo to validate and define alerts up front, before then usingadd_alert
to attach relevent ones to simulation, epoch or evaluation runs. The old way to do it would have only allowed the alerts to be validated when created and attached to a run, meaning eg you might spend hours training a model and then simvue crashes because the alert you defined for your evaluation run was invalid.Also spotted a couple of other bugs:
The
getter
forRunObject.alerts
returned a generator, but thesetter
expected a list - changed these to both be lists for consistency.In
add_alert
, there was a lineset(run.sv_obj.alerts + [id]
, whereid
was already a list - so removed the extraneous square bracketsadd_alert
would never work when passed a list of names since it still expected Alert.get to return the old dict format (instead of the current tuple format) - fixed this by adding [1]'s so that it would look at the alert element of the tupleget_alert_details()
will only work for online runs, so added a check if used when offline and raises an exception. Also added an 'if offline' check torun.alerts
so that it will just return the list of alert IDs, instead of doing a dict comprehension from the server returned infoadd_alerts
now always passes a full list of alerts which should be attached to the run at any time to the server endpoint, and the server endpoint has been changed to overwrite the list instead of appending to it (to stop duplicate alerts being added)Removed
_attach_alert_to_run()
- now just usesadd_alerts
Added test for
add_alerts
functionalityCloses #678
Closes #693
Closes #694
Closes #695
Closes #696
Closes #630 since
create_xyz_alert
can now be used with a disabled run andattach_to_run = False
to validate and create alerts up frontCloses #135