Support floats? #245
Comments
|
/cc @DirectXMan12 , number based fields should be all string? |
|
so, it's technically supported, but floats are highly discouraged (and disallowed in official kubernetes APIs) because they won't round-trip properly, can't actually represent certain numbers exactly, etc (all the normal reasons floats are usually not what you want in a program). We recommend using Closing this for now, but if you have a compelling argument, I'm happy to reconsider. |
|
I think it would be better to support it and have a warning about this in the documentation. There are scenarios where precision is not important and any workaround might be worse. |
|
it's not just precision -- it's mainly that it doesn't round-trip. Can you provide a concrete example here of a scenario where you want to use float? |
|
@DirectXMan12 We have an operator managing canary releases (using Istio). The user should be able to provide arbitrary prometheus queries with thresholds (usually a floating number) alongside the built-in metrics (request success rate and request duration). |
|
I think that's probably a better case for quantity than for a normal float, but I see your point. |
|
we could also have an "allowDangerousTypes" option too. |
|
@DirectXMan12 Thank you for considering it! From a UX perspective, by default I would log out a warning with a link to the relevant part of the documentation or a short description, but not fail to build. The |
|
/kind feature |
|
@DirectXMan12: Please ensure the request meets the requirements listed here. If this request no longer meets these requirements, the label can be removed In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
kind/feature
|
I am glad to share why we need float in CRD spec. We are implementing an AutoML (a.k.a Hyperparameter tuning) system using kubebuilder. CRD is here https://github.com/kubeflow/katib/tree/master/pkg/apis/controller. You can see https://github.com/kubeflow/katib/blob/master/pkg/apis/controller/common/v1alpha3/common_types.go#L42 where we use float We allow users to define the goal of the tuning experiment (e.g. When the accuracy reached the goal, we stop the experiment). And the goal is mainly a float number (e.g. 0.99 for accuracy, or 100.5 for loss and so on). Thus it will be better if we support floats. But it seems that we can have some hack around resource.Quantity |
|
@gaocegege it's not clear to me why that needs float in the IEEE sense. We could improve the syntax for resource.Quantity a bit, but I think resource.Quantity should suffice for your use case. |
|
Yeah, It also works. |
|
@DirectXMan12 Would you consider reopening this? We would be willing to contribute float support. We plan to include another CRD (like a template) in ours and that unfortunately uses a float64. |
|
Right now, it is unfortunately impossible to use |
Convert the Upstream health threshold and Target createdat values from float64 to string. Use of float64 doesn't allow controller-gen to automate manifests from these types, and using string instead is the typical approach in other projects: kubernetes-sigs/controller-tools#245
Convert the Upstream health threshold and Target createdat values from float64 to string. Use of float64 doesn't allow controller-gen to automate manifests from these types, and using string instead is the typical approach in other projects: kubernetes-sigs/controller-tools#245
Convert the Upstream health threshold and Target createdat values from float64 to json.Number Use of float64 prevents controller-gen from automatically generating maifests from these types: kubernetes-sigs/controller-tools#245
|
/reopen |
|
@alvaroaleman: Reopened this issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@alvaroaleman - FYI there's already some support for floats which I added in #449 behind a flag. |
Oh I wasn't aware of that, thanks for mentioning it! Maybe we could improve the handling to instead of falling through into |
the CRD generator doesn't support `float64` as a type of a CRD field. The sugested workaround is to used instead `resource.Quantity`, but we choose to use `string` for simplicity Related issue: kubernetes-sigs/controller-tools#245 Signed-off-by: cedric lamoriniere <[email protected]>
In the official docs I still see float support: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/, am I wrong?
I am currently hitting this error:
controller-tools/pkg/crd/schema.go
Line 387 in abcf7d8
Error I get is:
unsupported type "float32". I expected my field to be parsed and crd to have a field of typenumberin the yaml format.The text was updated successfully, but these errors were encountered: