SDK/DSL: Make 'name' argument of a PipelineVolume omittable #1402
Conversation
Also remove unused imports from _pipeline_volume module Signed-off-by: Ilias Katsakioris <elikatsis@arrikto.com>
| init_volume = {"name": kwargs.pop("name") | ||
| if "name" in kwargs else None} | ||
| init_volume = {"name": kwargs.pop("name") if "name" in kwargs | ||
| else "pvolume-%s" % id(self)} |
There was a problem hiding this comment.
It might be better to use a stable hash (e.g. SHA256) of serialized volume structure instead of id.
There was a problem hiding this comment.
The thing here is that we need to select a name in order to call super().__init__() which defines and fills the object's fields.
What does the structure contain before that? And if we do serialize the structure before the actual init(), wouldn't we get the same hash for every PipelineVolume? (since the input of the function will be the same empty structure)
There was a problem hiding this comment.
I understand that the designs where the ID/name is part of the structure make hashing problematic.
What I did was setting an empty id/name during creation, then hashing the structure and then setting the id/name based on the hash. Do you think this is appropriate here? The advantage is that the compiled pipeline is not changing with every rebuild.
There was a problem hiding this comment.
I implemented your suggestion.
First, I used a placeholder for the name.
Then, I used the to_dict() method the Kubernetes model has (I think there is no need to convert it the same way the compiler does, i.e. proper yaml representation).
Finally, I hashed the str() representation of that dictionary.
What do you think?
Also, take a look at the test. Since the name is the same, I assertEqual() with the name it should have, hard-coded. Should I leave it like this, delete the test, or perform the same actions in the test function (create a volume with the placeholder name and hash it) and then compare them?
There was a problem hiding this comment.
The test will sometimes fail since python dict is not ordered, thus the str(self.to_dict()) is not deterministic.
elikatsis
force-pushed
the
feat-omit-pipelinevolume-name
branch
from
4fe1fae
to
a58c304
Compare
|
I had made a silly mistake not maintaining the provided name. |
|
Hey @Ark-kun, could you go through this? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test kubeflow-pipeline-sample-test |
…oller (kubeflow#1402) * Validated that the inference service is ready otherwise throw error * Add test casee for when inference service is not ready * Added condition functions and condition InferenceServiceReady * Add check for trained model condition InferenceServiceReady * Set the condition for InferenceServiceReady * Updated status and returned if conditions are false * Add check for InferenceServiceReady condition in updating a trained model * newline * Add more ready conditions * Update conditions status after setting conditions * Removed trained model name as event is already on tm
This PR makes the creation of a
PipelineVolumewithout specifying anamepossible.The
nameof thevolumeis an orchestration detail [1] that the user should be able to be agnostic about.I also removed any unused imports from the
_pipeline_volumemodule.[1]: It is only needed for the matching of
volumeMounts with thevolumeentries.This change is