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

[alexlatchford]

As a part of some work I did this week (see #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]

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

• Ongoing burden on contributors to adhere to best practices, this may be particularly cumbersome if a decision is made to enable static analysis.
• Temporary burden on contributors whose work is in progress when we expose stylistic constraints.
• Cost to new contributors to learn how to use toolset.

Benefits

• Auto-formatters don't always play nice with linters but for the most part if we can make the two pieces work together we can reduce pull request code churn by assuring we all play by the same rules.
• Reduce ongoing code maintenance to offloading formatting tasks to IDEs (ie. vscode and/or pycharm both could integrate easily with yapf).
• Static analysis can find some bugs, but mostly it will drastically improve API documentation by enforcing type adherence.

---

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]

@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]

Some previous discussion: #4002 (comment)

[Bobgy]

For frontend code we enforced code style similar to the tradeoffs you described #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]

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

[alexlatchford]

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, #4002 (comment), 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]

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]

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

[Bobgy]

/reopen
/Lifecycle frozen

[k8s-ci-robot]

@Bobgy: Reopened this issue.

In response to this:

/reopen
/Lifecycle frozen

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.

[rimolive]

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

/close

[google-oss-prow]

@rimolive: Closing this issue.

In response to this:

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

/close

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.