[Project] Retain producers of exported artifacts #5283
Conversation
| producer = self._resolve_artifact_producer(item) | ||
| if producer.name != self.metadata.name: | ||
| # the artifact producer is retained, so log it only if it doesn't already exist | ||
| if existing_artifact := self._resolve_existing_artifact( |
There was a problem hiding this comment.
what if the artifact changed and we want to update it?
There was a problem hiding this comment.
That's a good question actually.
As per @theSaarco we shouldn't re-register artifacts if we retain their producer, but indeed the spec etc can change.
There was a problem hiding this comment.
Good question indeed.
Of course this is only expected if we're re-importing on the same env, since it's a run-id that already exists in the system. Even then, I tend to think it's a strange situation in which the same run-id had generated a different version of the same artifact. It seems more likely that the same artifact was modified after the run was done, in which case it would be probably better to maintain the manual changes done to the artifact.
I guess if the user wants to "reset" the artifact, it will need to be deleted before the import.
There was a problem hiding this comment.
ok, that makes sense. I suggest adding a log that says the artifact is already registered so we're skipping it.
| @@ -88,9 +88,10 @@ class ArtifactSpec(ModelObj): | |||
| "db_key", | |||
| "extra_data", | |||
| "unpackaging_instructions", | |||
| "producer", | |||
There was a problem hiding this comment.
Why was this change needed? This means the producer will now be in the result of base_dict() for artifacts - is this intended?
There was a problem hiding this comment.
When setting an artifact on the project it is using the base_dict(), so the producer property was not exported because it was a part of the _extra_fields.
This is a crucial part to maintain the producers when loading a project - to actually have it on the exported spec.
There was a problem hiding this comment.
So this means that this fix will not work for any project exported prior to this PR, right?
Also, need to make sure there are no BC issues there (I don't think there are, but needs to be verified).
There was a problem hiding this comment.
It obviously won't fix projects that were exported before this fix, so I'm not sure what BC issues we should check here..
There was a problem hiding this comment.
It's not obvious that projects exported before the fix won't be fixed - it's the situation because the export doesn't contain the information needed for the fix.
As for BC, just to make sure that now that artifact export/import contains the producer field in the base export fields, that artifacts that were exported before won't fail importing due to it (again, I'm pretty sure they won't fail, just something to pay attention to).
| producer = self._resolve_artifact_producer(item) | ||
| if producer.name != self.metadata.name: | ||
| # the artifact producer is retained, so log it only if it doesn't already exist | ||
| if existing_artifact := self._resolve_existing_artifact( |
There was a problem hiding this comment.
ok, that makes sense. I suggest adding a log that says the artifact is already registered so we're skipping it.
Previously, when artifacts where imported either via
load_projectorimport_artifact, theirproducerproperty (+tree) were overridden by the registering project information.In case artifacts were created by runs, overriding their
producercauses them to lose some information.To fix this, we retain the registered artifacts' producers according to the following logic:
Resolves https://iguazio.atlassian.net/browse/ML-4134