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

Add v1alpha1 XGBoostJob manifests #1113

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

terrytangyuan
Copy link
Member

@terrytangyuan terrytangyuan commented Apr 17, 2020

Signed-off-by: terrytangyuan [email protected]

Which issue is resolved by this Pull Request:
Resolves #1112

Description of your changes:

This adds the manifests for v1alpha1 XGBoostJob.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@kubeflow-bot
Copy link
Contributor

This change is Reviewable

@terrytangyuan terrytangyuan changed the title Add XGBoostJob manifests Add v1alpha1 XGBoostJob manifests Apr 17, 2020
@terrytangyuan
Copy link
Member Author

/cc @abhi-g @richardsliu @jlewi

@terrytangyuan
Copy link
Member Author

terrytangyuan commented Apr 17, 2020

I see this error when I execute make generate-changed-only: json: unknown field "env". This is caused by different versions of kustomize. Which of the following should I do?

  1. Modify the env in all the existing manifests to envs in a separate PR so that kubeflow/manifests could work with kustomize v3
  2. Use kustomize v2 when working with kubeflow/manifests instead

Related issues: kubernetes-sigs/kustomize#1069, kubeflow/metadata#202

@terrytangyuan
Copy link
Member Author

Skimming through the issues and it looks like we are migrating to kustomize v3 #1062. The problem here is that when I run generate_tests.py it will traverse through all the existing manifests. In other words, if any of them are not compatible with v3, I won't be able to generate tests for XGBoostJob successfully.

Any suggestions here? @abhi-g @richardsliu @jlewi

Signed-off-by: terrytangyuan <[email protected]>
@@ -0,0 +1,14 @@
apiVersion: kustomize.config.k8s.io/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

Application should not depend on base.
The application manifest is not overloading any resources defined in the base package.
So either

  1. create a single package which includes the application resource
    or
  2. Create 2 separate packages one with the base resources and one with the application resource
    and then create a 3rd package to update them.

@jlewi
Copy link
Contributor

jlewi commented Apr 21, 2020

@terrytangyuan I don't understand what the problem is. Which version of kustomize are you using?

I'm using

Version: {Version:kustomize/v3.2.1 GitCommit:d89b448c745937f0cf1936162f26a5aac688f840 BuildDate:2019-09-27T00:10:52Z GoOs:linux GoArch:amd64}

And I can run make generate off of master. So if make generate is failing for you then that suggests a problem with the version of kustomize you have or your pR.

@terrytangyuan
Copy link
Member Author

terrytangyuan commented Apr 22, 2020

@jlewi The problem is with kustomize v3.4. I downgraded it to v3.2.1 and it works now.

Regarding the application dependency on base, currently other operators all follow this same pattern. We can perhaps fix them altogether in a follow-up PR so this PR can focus on bringing XGBoostJob to kubeflow/manifests.

Could you take another look?

Signed-off-by: terrytangyuan <[email protected]>
@terrytangyuan
Copy link
Member Author

@jlewi Thank you for the detailed review! I've addressed your comments.

@jlewi
Copy link
Contributor

jlewi commented Apr 28, 2020

@terrytangyuan It looks like your application package is still depending on the base package.

I would suggest organizing it as follows

components/
    system - (rename base to system or something else more descriptive e.g. controller)
    application

installs
    full (not sure what a good name would be) - This would be a kustomize package that combines the system and application packages

The idea here is that installs contains fully deployable packages which are constructed by combining components.

You can take that as a suggestion though so if you prefer the current layout you can stick with it.

@terrytangyuan
Copy link
Member Author

@jlewi Thanks for the clarification. I'd like to keep the current layout for now as currently there are other operators that follow this pattern. We can perhaps address that altogether in the future so this PR can focus on bringing XGBoost operator available here.

@jlewi
Copy link
Contributor

jlewi commented Apr 29, 2020

/lgtm
/approve

@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

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.

Add manifests for XGBoostJob
5 participants