kubeflow / pipelines

Machine Learning Pipelines for Kubeflow
https://www.kubeflow.org/docs/components/pipelines/
Apache License 2.0
3.61k stars 1.63k forks source link

chore: Linting, auto-formatting & type checking for KFP SDK #4219

Closed alexlatchford closed 7 months ago

alexlatchford commented 4 years ago

As a part of some work I did this week (see https://github.com/kubeflow/pipelines/pull/4218) to clean up the SDK docs I noticed how variable the codebase standards for the Python SDK are. Looks like are some docs in the contribution guide that suggest a style, see here, and these choices seem perfectly reasonable to me.

My question is do we think it's worthwhile trying to enforce these standards? And, if so how do we want to migrate the existing code to use this?

I'm happy to volunteer to do the grunt work to make this happen but really I'd want agreement and buy-in as typically this would cause huge merge conflicts with any work in flight if we decided to do this.

alexlatchford commented 4 years ago

As with anything like this it requires a cost/benefit analysis, likely this is a well known problem to all but here's my best shot anyway.

Costs

Benefits


To go a little deeper my interest in contributing to Kubeflow is around how to make it easily accessible to Data Scientists without strong engineering fundamentals. Obviously this is a large goal but the barrier at the moment has not been a lack of functionality but rather the confusing nature of the model of KFP SDK concepts and cohesive documentation.

Bobgy commented 4 years ago

@numerology @Ark-kun I think you mentioned yapf can format only changed code, but I was unable to reproduce that feature on VSCode, everytime I try to format a file it just formats everything.

In the end, I'm unable to apply the new format when I make changes to python files.

Bobgy commented 4 years ago

Some previous discussion: https://github.com/kubeflow/pipelines/pull/4002#issuecomment-645182542

Bobgy commented 4 years ago

For frontend code we enforced code style similar to the tradeoffs you described https://github.com/kubeflow/pipelines/issues/2406, I was able to push a conclusion for all existing PRs and rush the code format enforcement without much interruption to others.

I couldn't figure out how to do formatting for python right now, so I hope there's either some guidance or we can enforce the format.

Bobgy commented 4 years ago

Of course, thanks @alexlatchford for driving these. Really appreciate your contribution!

alexlatchford commented 4 years ago

Some previous discussion: #4002 (comment)

Thanks for sharing this, looks like I need to convince @Ark-kun then that this a pain worth going through. In general I think I'm on same page as you mention here, https://github.com/kubeflow/pipelines/pull/4002#issuecomment-645180847, where this will have a period of short-term pain but on the whole will be worthwhile moving forward.

Really a goal of this was to add more type hints to help drive nicer SDK docs and I think that me contributing in that area is likely to be uncontroversial so long as we don't enable static type checking so maybe that's how I can start this work as we resolve the discussion in one direction or another 😄 Thanks for the guidance!

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 years ago

This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.

Bobgy commented 4 years ago

/reopen /Lifecycle frozen

k8s-ci-robot commented 4 years ago

@Bobgy: Reopened this issue.

In response to [this](https://github.com/kubeflow/pipelines/issues/4219#issuecomment-714328083): >/reopen >/Lifecycle frozen Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
rimolive commented 7 months ago

Closing this issue. No activity for more than a year.

/close

google-oss-prow[bot] commented 7 months ago

@rimolive: Closing this issue.

In response to [this](https://github.com/kubeflow/pipelines/issues/4219#issuecomment-2016821926): >Closing this issue. No activity for more than a year. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.