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 ekf distro #2641

Merged
merged 3 commits into from
Apr 23, 2021
Merged

Conversation

shannonbradshaw
Copy link
Contributor

Fixes #2639

cc: @RFMVasconcelos here's the PR for the Arrikto EKF description as discussed on #2611

@google-oss-robot
Copy link

Hi @shannonbradshaw. Thanks for your PR.

I'm waiting for a kubeflow 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@cvenets
Copy link

cvenets commented Apr 22, 2021

@RFMVasconcelos can you please merge the above PR today?

This is fully supported by Arrikto, tested and validated for 1.3, so it needs to be in the distributions table for people to be able to find it, before 1.3 gets announced.

@shannonbradshaw
Copy link
Contributor Author

shannonbradshaw commented Apr 22, 2021

@RFMVasconcelos we have a precedent for adding distros that members of the community are actively maintaining. Argoflow is just one example. It is arbitrary to exclude Arrikto EKF at this point. I'd asked to have this included in #2611 and you and @thesuperzapper asked me to file a separate PR. Here it is. I would appreciate getting this merged in time for the release announcement. I have not heard any specific objections to including this so there does not seem to be a reason to hold off.

On top of all that, given the effort on this release by the entire Arrikto team, I think we've earned the right to have Arrikto Enterprise Kubeflow listed in the distros table. It's up to date for 1.3. cc: @Bobgy

@rui-vas
Copy link
Contributor

rui-vas commented Apr 23, 2021

@shannonbradshaw to be honest, I feel uncomfortable making this decision on my own without a community agreement.

@shannonbradshaw
Copy link
Contributor Author

shannonbradshaw commented Apr 23, 2021

@RFMVasconcelos what, specifically, would you take as community agreement? Also, I'm not sure I understand the hesitation. We seem to have made the decision about Argoflow without community agreement.

@thesuperzapper
Copy link
Member

thesuperzapper commented Apr 23, 2021

I don't see any significant issues with including this distribution in the docs. Clearly, once we have resolve Kubeflow's governance issues, and then setup a distribution validation system, this list will surely change anyway.

@RFMVasconcelos @Bobgy from a logistical perspective one of you will have to /approve this.

But @shannonbradshaw please only make one change per PR, (don't bring the GCP thing in, as it's unrelated, and just makes it less likely to get merged)

Once @shannonbradshaw removes that other commit, I believe it's fine to merge.

EDIT: Just to clarify, as there is not really a governance body who can vote on this, unless there is significant objection within the committers, lets move forwards

This reverts commit 0eacee5.
@shannonbradshaw
Copy link
Contributor Author

@thesuperzapper @RFMVasconcelos @Bobgy ok I reverted the last commit per @thesuperzapper's request.

@jbottum
Copy link
Contributor

jbottum commented Apr 23, 2021

/approve

@thesuperzapper
Copy link
Member

Do you really need to add the AWS acronym @shannonbradshaw ?

@thesuperzapper
Copy link
Member

/ok-to-test
/hold

Holding for the AWS acronym

@shannonbradshaw
Copy link
Contributor Author

@thesuperzapper if we use the acronym it should be defined. That is a docs best practice.

@andreyvelich
Copy link
Member

From my point of view, we should include all distribution in the official Kubeflow docs that vendors support.
Until we don't have a clear vision on how to explain users how to use Kubeflow on various platforms, www.kubeflow.org/docs/distributions/ should be source of truth.

There are couple of advantages by doing this for Kubeflow 1.3:

  1. Users will have more variety to deploy Kubeflow.
  2. Kubeflow will be more popular and stable if we test it in the different environments.
  3. Community will get support from the Cloud/Platform vendors.

I don't see any problems with merging this PR.

/lgtm
cc @RFMVasconcelos

@rui-vas
Copy link
Contributor

rui-vas commented Apr 23, 2021

Thank you for your comment @andreyvelich! Given your support + @thesuperzapper and @Bobgy's ack, I think I can pull the trigger to add this distribution without upsetting anyone in the community.

Apologies for the delay on this @cvenets @shannonbradshaw @castrojo @jbottum, having more people chime in was important for me to be making this call.

/lgtm
/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbottum, RFMVasconcelos, shannonbradshaw

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

@shannonbradshaw
Copy link
Contributor Author

/hold cancel

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 Arrikto EKF distribution
7 participants