(Fix): Change naming to be inline with kubernetes naming #310
Conversation
There was a problem hiding this comment.
Hi @davidspek, thanks for the contribution!
I left a small review comment
FAQ.md
Outdated
| @@ -35,7 +35,7 @@ What you can do in this case is either install a storage class that enables | |||
|
|
|||
| 1. Retrieve the `.py` file generated by Kale (it should be next to the `.ipynb`) | |||
| 2. Search for `marshal_vop` definition (`marshal_vop = dsl.VolumeOp...`) | |||
| 3. Change this line `modes=dsl.VOLUME_MODE_RWM`, to `modes=dsl.VOLUME_MODE_RWO` | |||
| 3. Change this line `modes=dsl.VOLUME_MODE_RWX`, to `modes=dsl.VOLUME_MODE_RWO` | |||
There was a problem hiding this comment.
Could you revert this, since it refers to https://github.com/kubeflow/pipelines/blob/91c5a93c8ed3caa49de4a23d596dff2d22cc6015/sdk/python/kfp/dsl/_volume_op.py#L30-L32 ?
Or, even better, since this seems to be a bit outdated, could you change it to
3. Change this line `modes=['ReadWriteMany']`, to `modes=['ReadWriteOnce']`
There was a problem hiding this comment.
I added the change you suggested. However, I'm not sure if this change will break things overall if it is referring to kfp. Seems like the KFP SDK needs to be updated so it follows the Kubernetes naming convention as well.
There was a problem hiding this comment.
The rest of the changes are internal to Kale so we are OK.
Changing the KFP DSL may be trickier and require some transitioning period, since VOLUME_MODE_* constants have been there quite some time.
I saw that in your fixup you deleted item (4) of the list, could you fix this so we merge?
There was a problem hiding this comment.
My bad, should be good now
There was a problem hiding this comment.
KFP DSL will indeed be difficult, but I think they should have used the Kubernetes naming convention from the start then.
davidspek
force-pushed
the
fix-access-mode-naming
branch
from
c33ab74 to
b0ef77e
Compare
The abbreviation used for ReadOnlyMany (ROM) and ReadWriteMany (RWM) in the code are not inline with the naming used by Kubernetes. It is best to adhere to the standard naming convention, which is ROX and RWX for ReadOnlyMany and ReadWriteMany respectively.
https://kubernetes.io/docs/concepts/storage/persistent-volumes/