Skip to content

Conversation

@ScorpioCPH
Copy link
Member

@ScorpioCPH ScorpioCPH commented Jan 2, 2018

@jlewi @gaocegege @zjj2wry @wackxu @DjangoPeng This PR proposed new directories layout as shown below:

$ tree -d -I 'vendor|bin|.git'
├── build
│   ├── images
│   └── release
├── cmd
│   └── tf_operator
├── dashboard
│   ├── backend
│   └── frontend
├── docs
│   └── diagrams
├── examples
│   ├── charts
│   ├── gke
│   ├── tensorflow-models
│   ├── tf_job
│   └── tf_sample
├── hack
│   └── py
├── pkg
│   ├── apis
│   │   └── tensorflow
│   ├── client
│   ├── controller
│   └── util
├── test
│   ├── e2e
│   └── test-infra
└── version

PTAL and leave your comments about this change, and after discussion i will send a PR to do this directories refactor. Thanks!


This change is Reviewable

@k8s-ci-robot
Copy link

Hi @ScorpioCPH. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage increased (+0.08%) to 28.601% when pulling 0dcf6e6 on ScorpioCPH:proposed-dic-layout into ed7cae7 on tensorflow:master.

@zjj2wry
Copy link
Member

zjj2wry commented Jan 2, 2018

/ok-to-test

@zjj2wry
Copy link
Member

zjj2wry commented Jan 2, 2018

mv version to pkg would be better.

@jlewi
Copy link
Contributor

jlewi commented Jan 3, 2018

Why is py under hack? It contains a bunch of scripts that are critical to our test infrastructure.
I think my preference would be to put all of our python scripts under py and start organizing it into packages
e.g.
py
tfk8s
testing
....
hack

So that one could do

import tfk8s.testing
import  tfk8s.hack

I don't think the tf-job-operator chart belongs under examples; its not an example its the actual way we recommend deploying the tf-job-operator (although I want to switch to ksonnet).

Does it make more sense to leave the charts directory and move it into that?

@ScorpioCPH
Copy link
Member Author

@jlewi Hi, I follow up Kubernetes style :)

It contains a bunch of scripts that are critical to our test infrastructure.

How about put py into test?

its not an example its the actual way we recommend deploying the tf-job-operator.

Move them into build/release?

@jlewi
Copy link
Contributor

jlewi commented Jan 5, 2018

Sorry for the delay

How about put py into test?

Can we leave the py directory as is for now?

The modules aren't necessarily test specific so they might not belong in testing. For example, there's a very crude python client library for working with tfjobs. This is useful outside of testing; e.g. for managing TfJobs from Jupyter notebooks. So eventually we might want to turn this into a pip package that we officially release maintain.

So I think the direction we are headed is organizing all py code under "/py" such that anything in "py" is a top level package.

its not an example its the actual way we recommend deploying the tf-job-operator.
Move them into build/release?

./tf-job-operator-chart isn't the actual release artifact. The released artifact is the result of substituting in values to pin things like images to a particular release. So I don't know that it makes sense to put in build/release.

That said; I'd like to get rid of the helm packages in favor of ksonnet (#239). So I'd suggest leaving it where is for now since we'll eventually delete it so moving it is just churn.

@jlewi
Copy link
Contributor

jlewi commented Jan 5, 2018

I'd also be ok with putting the tf-job-operator chart under hack or /legacy or /deprecated to indicate its something we want to get rid of.

@gaocegege
Copy link
Member

gaocegege commented Jan 5, 2018

The python code is uncoupled with the go code so could we place the python code into another repo?

@ScorpioCPH
Copy link
Member Author

So I think the direction we are headed is organizing all py code under "/py" such that anything in "py" is a top level package.

I'd also be ok with putting the tf-job-operator chart under hack or /legacy or /deprecated to indicate its something we want to get rid of.

LGTM :)

$ tree -d -I 'vendor|bin|.git'
├── build
│   ├── images
│   │   └── tf_operator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is in the images directory? Dockerfiles for images?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

│   └── tf_sample
│   └── tf_sample
├── hack
│   └── py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion please make py toplevel directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

├── examples
│   ├── charts
│   │   ├── tensorboard
│   │   └── tf-job-operator-chart
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per conversation tf-job-operator-chart should not be under examples.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ScorpioCPH ScorpioCPH force-pushed the proposed-dic-layout branch from 0dcf6e6 to b104cf9 Compare January 5, 2018 07:18
@coveralls
Copy link

coveralls commented Jan 5, 2018

Coverage Status

Coverage increased (+0.08%) to 28.458% when pulling b104cf9 on ScorpioCPH:proposed-dic-layout into f7803f1 on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 5, 2018

Approved

@jlewi jlewi merged commit 5fdfdbf into kubeflow:master Jan 5, 2018
jlewi pushed a commit that referenced this pull request Jan 16, 2018
As proposed here: #261

This PR move folders around to match new directories layout with nothing changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants