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

Development Environment Setup Guide for Flyte Components #3811

Merged
merged 14 commits into from
Jul 14, 2023

Conversation

Yicheng-Lu-llll
Copy link
Member

@Yicheng-Lu-llll Yicheng-Lu-llll commented Jun 28, 2023

Describe your changes

Considering the rapid development and updates in Flyte's components, including flyteidl, flyteadmin, flyteplugins, flytepropeller, flytekit, and flyteconsole, existing guides may not fully reflect the current state of the project. This PR introduces a comprehensive Development guide for new contributors to Flyte.

The guide provides a step-by-step approach to setting up a local development environment for nearly all Flyte components, namely flyteidl, flyteadmin, flyteplugins, flytepropeller, flytekit, and flyteconsole.

Many in the open-source community might be eager to contribute but find the initial setup process a daunting hurdle. I too struggled with the setup when I first started contributing (many thanks to Kevin Su for all his help!). To break down these barriers and make the contribution process more accessible, I've created this guide.

Feedback and suggestions are highly appreciated!!!!!!

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Note to reviewers

Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
@bstadlbauer
Copy link
Member

Thank you @Yicheng-Lu-llll, this is great! I remember struggling a lot to get a first version of a development setup as well!

Should we merge this with the existing guide to avoid duplication?

@fg91 fg91 self-requested a review June 28, 2023 07:28
@Yicheng-Lu-llll
Copy link
Member Author

Should we merge this with the existing guide to avoid duplication?

Sure, Will merge with the existing guide!

Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
@gpgn
Copy link

gpgn commented Jun 30, 2023

I'm following this guide to set up a local dev environment. Had to run with sudo -E env "PATH=$PATH" make compile to compile the flyte single binary. Running it without sudo gives an error:

/bin/sh: 1: go: not found
make: *** [Makefile:21: compile] Error 127

@Yicheng-Lu-llll
Copy link
Member Author

Yicheng-Lu-llll commented Jun 30, 2023

Hi @gpgn, it appears that the Go isn't included in your PATH . Would you mind adding to your PATH and then run the go version command to verify? Correct me if this is not your case.

@gpgn
Copy link

gpgn commented Jun 30, 2023

It was in my PATH, but not for the root user. Using -E env "PATH=$PATH fixes it.

Now running into an issue where it can't find $HOME/.flyte/webhook-certs:

{"json":{"src":"start.go:185"},"level":"panic","msg":"Failed to start Propeller, err: mkdir $HOME/.flyte/webhook-certs: no such file or directory","ts":"2023-06-30T15:29:15+02:00"}

Raised it here: https://flyte-org.slack.com/archives/CP2HDHKE1/p1688129196715809

@Yicheng-Lu-llll
Copy link
Member Author

Yicheng-Lu-llll commented Jun 30, 2023

Hi, @gpgn, Are you using flyte_local.yaml in #3808? The old flyte_local.yaml will not work. I have the following comment in the doc:

# Note: Replace `flyte_local.yaml` with file in this PR:https://github.com/flyteorg/flyte/pull/3808. Once it is merged, there is no need to change.

pingsutw
pingsutw previously approved these changes Jul 6, 2023
Copy link
Contributor

@hamersaw hamersaw left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thanks so much for breaking this down. Left a few comments to help clarify a few points and hopefully generalize this a little.

rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
rsts/community/contribute.rst Outdated Show resolved Hide resolved
Signed-off-by: Yicheng-Lu-llll <[email protected]>
@samhita-alla
Copy link
Contributor

This is amazing, @Yicheng-Lu-llll! Really appreciate you taking the time to document the setup of the development environment.

@samhita-alla
Copy link
Contributor

@hamersaw, can we merge this?

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

Yicheng-Lu-llll commented Jul 13, 2023

Hi @samhita-alla, I've reviewed and rerun the code again, confirming that it operates without any errors.
I just slightly enhanced the language and structure in the last commit.

@hamersaw
Copy link
Contributor

@hamersaw, can we merge this?

Approved by me! Was hoping to have a few more eyes on this from flytekit / flyteconsole contributors. But this is a great improvement over what we currently have (or better said "don't have"). Thanks @Yicheng-Lu-llll .

@samhita-alla
Copy link
Contributor

Kevin approved the changes already. Will merge the PR.

@samhita-alla samhita-alla merged commit 3d3d72d into flyteorg:master Jul 14, 2023
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