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.

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

option naming inconsistencies #303

Closed
fxue opened this issue Feb 27, 2018 · 2 comments
Closed

option naming inconsistencies #303

fxue opened this issue Feb 27, 2018 · 2 comments

Comments

@fxue
Copy link
Contributor

fxue commented Feb 27, 2018

In kubeflow-core options are named camel case e.g. "jupyterHubServiceType"
but in tf-serving options are name with under scores e.g. "http_proxy_image"
We should have consistent naming standard, should we adopt under score naming schemes for all three?

@jlewi
Copy link
Contributor

jlewi commented Mar 8, 2018

It looks like the convention in ksonnet is camel case
https://github.com/ksonnet/parts/blob/master/incubator/mysql/prototypes/simple-mysql.jsonnet
https://github.com/ksonnet/parts/blob/master/incubator/redis/prototypes/redis-all-features.jsonnet

I suggest we follow that. I'll fix this for tfServing as part of #387.

We should probably document this in a style guide somewhere.

jlewi added a commit to jlewi/kubeflow that referenced this issue Mar 8, 2018
jlewi added a commit to jlewi/kubeflow that referenced this issue Mar 8, 2018
…clouds.

* To support GPUs and specific clouds we refactor the component to make
  it easy to override the parts we care about (e.g. container environment
  variables, resources, etc...).

* We do this by moving the things we care about up to the root of tf-serving.libsonnet.

* We rely on jsonnet late binding (http://jsonnet.org/docs/tutorial.html).
Late binding allows us to devine dictionaries (e.g. params, tfServingContainer)
in tf-serving.libsonnet. We can then create manifests based on those
objects (e.g. tfDeployment). We can then override values (e.g. params) and
the derived objecs (e.g. tfDeployment) will use the overwritten values.

* We introduce a parameter "cloud" which allows us to control which "prototype" to use. We use this to use cloud specific customizations; like setting
  the environment variables on AWS to use S3.

* Late binding also makes it possible to select an appropriate default image
  based on whether GPUs are bing used or not while still allowing the
  user to override the images.

* We remove parameter definitions from the prototypes. The set of parameters
  ends up being conditional based on flags like cloud, GPUs so its
  not clear how scalable that was.

* Use camelCase not underscores for parameters.
  See kubeflow#303.

Related Issues:

Fix kubeflow#292

Update the test to work with the changes.

* Parameters are now camelCase. They also aren't parameters of the
  prototype so we can't set them in the call to generate.

* So we need to modify deploy to take a list of the parameters to set
  on the component.
k8s-ci-robot pushed a commit that referenced this issue Mar 8, 2018
…clouds (#387)

* Refactor the TFServing component to better support GPUs and specific clouds.

* To support GPUs and specific clouds we refactor the component to make
  it easy to override the parts we care about (e.g. container environment
  variables, resources, etc...).

* We do this by moving the things we care about up to the root of tf-serving.libsonnet.

* We rely on jsonnet late binding (http://jsonnet.org/docs/tutorial.html).
Late binding allows us to devine dictionaries (e.g. params, tfServingContainer)
in tf-serving.libsonnet. We can then create manifests based on those
objects (e.g. tfDeployment). We can then override values (e.g. params) and
the derived objecs (e.g. tfDeployment) will use the overwritten values.

* We introduce a parameter "cloud" which allows us to control which "prototype" to use. We use this to use cloud specific customizations; like setting
  the environment variables on AWS to use S3.

* Late binding also makes it possible to select an appropriate default image
  based on whether GPUs are bing used or not while still allowing the
  user to override the images.

* We remove parameter definitions from the prototypes. The set of parameters
  ends up being conditional based on flags like cloud, GPUs so its
  not clear how scalable that was.

* Use camelCase not underscores for parameters.
  See #303.

Related Issues:

Fix #292

Update the test to work with the changes.

* Parameters are now camelCase. They also aren't parameters of the
  prototype so we can't set them in the call to generate.

* So we need to modify deploy to take a list of the parameters to set
  on the component.

* jsonnet format.
@jlewi
Copy link
Contributor

jlewi commented Mar 9, 2018

TFServing Fixed by #387

@jlewi jlewi closed this as completed Mar 9, 2018
yanniszark pushed a commit to arrikto/kubeflow that referenced this issue Feb 15, 2021
* add api doc

Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>

* fix typo

Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>

* add instructions for update api files and docs

Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>

* fix typo

Signed-off-by: YujiOshima <yuji.oshima0x3fd@gmail.com>
elenzio9 pushed a commit to arrikto/kubeflow that referenced this issue Oct 31, 2022
Please consider adding @nicholas-abad to the organisation. He may not be as visibly active in the Kubeflow community as I am, but is doing plenty of heavy lifting in the backgroud. He will help me with Model Management topics, and has also (as confirmed by him in kubeflow/community#342) volunteered to act as a calendar admin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants