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

Fix for issue 1050 - camelCase for some recently fixed params. #1556

Merged
merged 3 commits into from Sep 19, 2018

Conversation

ashahba
Copy link
Member

@ashahba ashahba commented Sep 18, 2018

fixes #1050
First round of changes include updating image_gpu, num_schedulers, num_servers, num_workers, num_gpus and giturl in files:

kubeflow/mxnet-job/prototypes/mxnet-job.jsonnet
kubeflow/pytorch-job/prototypes/pytorch-job.jsonnet
kubeflow/weaveflux/README.md
kubeflow/weaveflux/all.libsonnet
kubeflow/weaveflux/prototypes/weaveflux.jsonnet

More is on the way but I need the get green light on this first before making the rest of changes that I mentioned in here: #1050 (comment)

Otherwise this should be sufficient for the release and I can file a separate issue and a subsequent PR to work on that after we are done with the release.

I'm open to either one, please let me know.


This change is Reviewable

…m_schedulers, num_servers, num_workers, num_gpus and giturl.
@jlewi
Copy link
Contributor

jlewi commented Sep 18, 2018

/lgtm
/approve

Would be great if you could do a quick check of the docs and see if they need to be updated.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ashahba
Copy link
Member Author

ashahba commented Sep 18, 2018

@jlewi I did a deep search within the repo and to best of my knowledge the only param that needed a doc/README.md update is giturl which is already reflected here: https://github.com/kubeflow/kubeflow/pull/1556/files#diff-d69b704385e0955a4336e131a3c802b8R18

If there are other places where we keep the docs, please let me know.

@jlewi
Copy link
Contributor

jlewi commented Sep 19, 2018

@ashahba Thanks I was specifically referring to the docs on our website
www.kubeflow.org
which are in kubeflow/website
Can you take a look at the docs for the components updated in this PR and see if the docs need to be updated e.g.
https://www.kubeflow.org/docs/guides/components/mxnet/

@jlewi
Copy link
Contributor

jlewi commented Sep 19, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot merged commit faea44a into kubeflow:master Sep 19, 2018
@ashahba
Copy link
Member Author

ashahba commented Sep 19, 2018

Thanks @jlewi .
It looks like the only match we have in the website code, is in this file:
tftraining.md
and for num_gpus.
But those are container args for Ps and worker that are consumed by Python training scripts and they are still valid and won't be effected by this change, so no doc update is needed in this case unless I'm missing something.

Please let me know if that sounds reasonable 🙂

@ashahba ashahba deleted the ashahba/fix-1050 branch September 27, 2018 20:25
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
…low#1556)

* camelCase for some recently fixed params, these include image_gpu, num_schedulers, num_servers, num_workers, num_gpus and giturl.

* Style enhancements for pytorch-job.jsonnet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

camelCase for some recently fixed params
4 participants