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.

Sign up for GitHub

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

Jump to bottom

[Project] Add delete artifact method #5491

Merged
merged 3 commits into from
May 7, 2024
Merged

[Project] Add delete artifact method #5491

merged 3 commits into from
May 7, 2024

Conversation

moranbental
Copy link
Member

Creating a new method in MlrunProject that allows the user to delete an artifact object in the DB and optionally delete the artifact data.

artifact = project.get_artifact("test-delete")
project.delete_artifact(artifact, deletion_strategy=mlrun.common.schemas.artifact.ArtifactsDeletionStrategies.data_optional, secrets={"secret1": "user-secret"})

https://iguazio.atlassian.net/browse/ML-6295

@moranbental moranbental changed the title [Project] Add delete artifact thorough project [Project] Add delete artifact method thorough project May 2, 2024
@moranbental moranbental changed the title [Project] Add delete artifact method thorough project [Project] Add delete artifact method May 2, 2024
alonmr
alonmr reviewed May 6, 2024
View reviewed changes
Copy link
Member

@alonmr alonmr left a comment

Choose a reason for hiding this comment

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

A few comments / questions

mlrun/db/httpdb.py
"""Delete an artifact.

:param key: Identifying key of the artifact.
:param tag: Tag of the artifact.
:param project: Project that the artifact belongs to.
:param tree: The tree which generated this artifact.
:param uid: A unique ID for this specific version of the artifact (the uid that was generated in the backend)
:param deletion_strategy: The artifact deletion strategy types.
Copy link
Member

Choose a reason for hiding this comment

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

point to where they can see the types

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? The parameter type exists in the method signature

mlrun/projects/project.py
"""Delete an artifact object in the DB and optionally delete the artifact data

:param item: Artifact object (can be any type, such as dataset, model, feature store).
:param deletion_strategy: The artifact deletion strategy types.
Copy link
Member

Choose a reason for hiding this comment

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

same

mlrun/common/schemas/artifact.py Outdated Show resolved Hide resolved
@moranbental moranbental requested a review from alonmr May 7, 2024 05:43
alonmr
alonmr approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@alonmr alonmr left a comment

Choose a reason for hiding this comment

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

Approved but I still think it's better to be explicit in the docstring (remember it also goes to mlrun docs site). See mlrun.db.httpdb.HTTPRunDB.delete_project for reference.

@alonmr alonmr merged commit 913f02a into mlrun:development May 7, 2024
10 checks passed
@moranbental moranbental deleted the delete-artifact-data-thorough-project branch May 7, 2024 12:14
assaf758 pushed a commit that referenced this pull request May 21, 2024
@alxtkr77 @moranbental
@jillnogold @gtopper @yanburman @quaark @liranbg @Eyal-Danieli @alonmr @roei3000b @rokatyy @jond01 @TomerShor @daniels290813 @ZeevRispler
[Spark] Enable support for non-aggregation operations in Spark using …
364839d 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liranbg liranbg liranbg approved these changes

@alonmr alonmr alonmr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@moranbental @liranbg @alonmr