-
Notifications
You must be signed in to change notification settings - Fork 241
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
[Alerts] Modify alert entity to have list of ids per alert #5511
Conversation
bcea382
to
0e2b87c
Compare
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.
Some pretty minor comments, but let's make sure our naming and terminology is consistent.
Signed-off-by: Yan Burman <yanburman@users.noreply.github.com>
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.
LGTM
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.
I don't really understand this PR, and I feel it is adding instability to the code without adding any functionality at all.
Why would we change the id/entity to a list of ids/entities which we validate to always be a length of 1, and any time we access it we always access ids[0]
?
Let's either actually extend the functionality to work with lists or stay with single entity, this "preparation" adds complexity and instability in my honest opinion. (And I'll also add that the PR should explain the rationality of the change as well)
@theSaarco as you approved this PR, maybe you could comment here as well?
This is to prepare for future stages where we will as support for multiple entities in a single alert. We want to be ready without having to modify the data structures and go with BC changes. |
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.
Spoke with @theSaarco and @yanburman offline and understood the magnitude of implementing multiple entity support is out of scope at the current release, so we will keep the preparation for the next release.
@yanburman please add some info on the plans of the multiple entity support in the PR description.
…the streaming interface (#5599) * [Datastore] Rename `DatastoreProfileAzureBlob` 'bucket' parameter to 'container' (#5521) * [Project] Add delete artifact method (#5491) * [API] Add delete artifact data endpoints (#5477) * [Docs] Add registering an HDFS datastore profile (#5396) To * how to register a datastore profile * Update docs/store/datastore.md Co-authored-by: Gal Topper <gal.topper@gmail.com> * Update docs/store/datastore.md Co-authored-by: Gal Topper <gal.topper@gmail.com> * fix: data store is 2 words * clarify example --------- Co-authored-by: Gal Topper <gal.topper@gmail.com> * [Alerts] Add backwards compatibility for igz < 3.6 (#5524) * [Coding Conventions] Fix `mlconf` usages (#5525) * [API] Enhance usage of iguazio client (#5506) * [Model Monitoring] Implement TSDB abstraction (#5210) * [Automation] Don't rollout deployment before purging DB (#5527) * [SQLDB] Refactor partition querying to minimize subquery table size (#5526) * [Docs] Enable automodule for mlrun.comon.schemas.artifact (#5529) * [Alerts] Filter project resources by permissions for list (#5532) * [ProjectSummaries] Add pipelines_completed_recent_count, pipelines_failed_recent_count (#5523) * [KFP] Merge KFP2 Feature Branch (#5533) * [Nuclio] Delete remote function config map (#5530) * [API] Add read model monitoring metrics values endpoint (#5519) * [Model Monitoring] Fix up imports (#5531) * Remove unused import * Change import so that PyCharm detects inheritance correctly * [Notifications] Display Step Kind In Pipeline Notification (#5538) * Add step kind to pipeline notification * better system test * redundant comment * fix ut --------- Co-authored-by: quaark <a.melnick@icloud.com> * [Alerts] Modify alert entity to have list of ids per alert (#5511) * [Application] Fix test function handler assertion (#5548) * [Docs] Feature-store + Datastore: consolidate Apache Kafka details, cleanups in datastores (#5305) * [Model Monitoring] Fix invalid current_stats in `MonitoringApplicationContext` (#5552) fix deepcopy bug * [KFP] Bump version and create makefile for release (#5555) * [Runs] Store artifact URIs instead of full artifacts in body (#5553) * [Artifacts] Add `producer_uri` filter to list artifacts API (#5549) * [API] Check the artifact kind while attempting to remove artifact data (#5545) * [CI] Fix pipeline adapters makefile (#5557) * [Model Monitoring] Update model endpoint's current stats and drift measures in the writer (#5554) * [Model Monitoring] Add new TSDB target for all predictions (#5551) * [Model Monitoring] Add new TSDB target for all predictions Tracking count and latency. [ML-6349](https://iguazio.atlassian.net/browse/ML-6349) * Change TSDB predictions table path * Move predictions table * Fix * Fix parameter names * Fix * [Functions] Update function with deletion task id (#5536) * [Application] Create API Gateway by default (#5515) * [Logger] Enhance formatter creation (#5561) * [KFP] Suppress reusable-components warnings (#5501) * [ProjectSummaries] Add distinct_scheduled_jobs_pending_count, distinct_scheduled_pipelines_pending_count (#5535) * [Notifications] Fix Pipeline Notification Log Spam + Git Notification (#5563) fix pipeline notification bugs Co-authored-by: quaark <a.melnick@icloud.com> * [Docs] Compile notification when compiling docs (#5564) * [Helpers] Ignore pre-release in `min_iguazio_version` (#5558) * [Runs] Enrich run with artifacts when getting a single run (#5565) * [Application] Set api gateway name on status (#5567) * [CI] Bundling demos as release asset (#5550) * [Nuclio-jupyter] Bump nuclio-jupyter to 0.9.17 (#5568) * [FrontendSpec] Adding TTLCache to `try_get_grafana_service_url` function (#5543) * [KFP] Fix `watch=True` for remote pipeline when using `set_workflow` (#5572) * [Alerts] Add cooldown_period to notification config in alert (#5547) * [Model] Replace yaml dump with safe_dump (#5571) * [Versioning] Fix package version for dev feature branches (#5575) * repace hasattr() with dir() * Support single Parquet and CSV files --------- Co-authored-by: moranbental <107995850+moranbental@users.noreply.github.com> Co-authored-by: jillnogold <88145832+jillnogold@users.noreply.github.com> Co-authored-by: Gal Topper <gal.topper@gmail.com> Co-authored-by: Yan Burman <yanburman@users.noreply.github.com> Co-authored-by: Adam <adamm@iguazio.com> Co-authored-by: Liran BG <liranbg@users.noreply.github.com> Co-authored-by: Eyal Danieli <eyald@iguazio.com> Co-authored-by: Alon Maor <48641682+alonmr@users.noreply.github.com> Co-authored-by: roei3000b <40743125+roei3000b@users.noreply.github.com> Co-authored-by: Katerina Molchanova <35141662+rokatyy@users.noreply.github.com> Co-authored-by: Jonathan Daniel <36337649+jond01@users.noreply.github.com> Co-authored-by: quaark <a.melnick@icloud.com> Co-authored-by: TomerShor <90552140+TomerShor@users.noreply.github.com> Co-authored-by: daniels290813 <78727943+daniels290813@users.noreply.github.com> Co-authored-by: ZeevRispler <73653682+ZeevRispler@users.noreply.github.com> Co-authored-by: Alex Toker <alex_toker@mckinsey.com>
https://iguazio.atlassian.net/browse/ML-6369
Note that we break backward compatibility here, but the original endpoint was added in 1.7 and not released yet, so nobody is using it
We add an ability to have list of ids, as in the future (1.8 ?) we want to have 2 more modes of operation where we allow multiple ids as either a single group or as a helper to manage alert logic per id