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.

Sign up for GitHub

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

Jump to bottom

docs(contribute): rewrite flyte contribute docs based on 4960 #5260

Merged
merged 3 commits into from
Nov 13, 2024
Merged

docs(contribute): rewrite flyte contribute docs based on 4960 #5260

merged 3 commits into from
Nov 13, 2024

Conversation

bearomorphism
Copy link
Contributor

@bearomorphism bearomorphism commented Apr 21, 2024 •
edited
Loading

Tracking issue

NA

Why are the changes needed?

The contribution guide of flyte can be improved.

What changes were proposed in this pull request?

Rewrote the contribution guide of flyte.

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

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

Related PRs

#4960

Docs link

@welcome Welcome
Copy link

welcome bot commented Apr 21, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. documentation Improvements or additions to documentation labels Apr 21, 2024
@codecov Codecov
Copy link

codecov bot commented Apr 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.95%. Comparing base (be67457) to head (48db2cb).
Report is 5 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #5260   +/-   ##
=======================================
  Coverage   36.94%   36.95%           
=======================================
  Files        1310     1310           
  Lines      131401   131401           
=======================================
+ Hits        48549    48555    +6     
+ Misses      78637    78632    -5     
+ Partials     4215     4214    -1
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.07% <ø> (ø)
unittests-flytecopilot 22.23% <ø> (ø)
unittests-flytectl 62.39% <ø> (ø)
unittests-flyteidl 6.95% <ø> (ø)
unittests-flyteplugins 53.84% <ø> (ø)
unittests-flytepropeller 43.08% <ø> (+0.02%) ⬆️
unittests-flytestdlib 55.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jasonlai1218
jasonlai1218 previously approved these changes Apr 21, 2024
View reviewed changes
Copy link
Contributor

@jasonlai1218 jasonlai1218 left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 21, 2024
neverett
neverett reviewed Apr 22, 2024
View reviewed changes
docs/community/contribute.rst Outdated

* Install `conda-lock <https://github.com/conda/conda-lock>`__.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave these steps in as an alternative to the steps below? You can move them down and put them in a "Alternative conda-lock setup steps" section.

Copy link
Contributor Author

@bearomorphism bearomorphism Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, but I am not sure how to build the Flyte doc by simply installing conda-lock. I guess this is only relevant to #4862, and we have to revert part of #4960 in order to make the alternative work.

@MortalHappiness Is conda-lock needed by make dev-docs?

Copy link
Member

Choose a reason for hiding this comment

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

@bearomorphism No. conda-lock is not needed by using make dev-docs because it is already installed in the Dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add the following steps back as an alternative.

flyte/docs/community/contribute.rst

Lines 156 to 164 in 84c0fef

To build the Flyte docs locally you will need the following prerequisites:
* Install ``conda``.
* We recommend Miniconda installed with an `official installer <https://docs.conda.io/projects/miniconda/en/latest/index.html#latest-miniconda-installer-links>`__.
* Install `conda-lock <https://github.com/conda/conda-lock>`__.
* In the ``flyteorg/flyte`` root directory run:
* ``conda-lock install --name monodocs-env monodocs-environment.lock.yaml``
* ``conda activate monodocs-env``
* ``pip install ./flyteidl``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

neverett
neverett previously approved these changes Apr 24, 2024
View reviewed changes
@Mecoli1219
Copy link
Contributor

@bearomorphism This PR is really helpful! I followed the old document build guides on my M2 Macos and got really frustrated. Could someone help push this PR to improve the current contribution guide?

@Future-Outlier
Copy link
Member

Hi, @bearomorphism
We can get this merged, can you solve the conflict problem?

@Future-Outlier Future-Outlier enabled auto-merge (squash) November 12, 2024 14:21
wild-endeavor
wild-endeavor approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

We can merge this, but can we add at some point instructions that don't involve conda? I like using uv, and i think it's plenty fast. If conda is not required, can we reword "We recommend Miniconda..."

docs/community/contribute/contribute_code.rst

**Setup process**

1. First you need to make sure you can run linux/amd64 container
Copy link
Contributor

Choose a reason for hiding this comment

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

Can arm macs by default run amd64 images? Not well right? I feel like I get errors all the time. Is the image multi-arch?

@Future-Outlier Future-Outlier merged commit f27aff4 into flyteorg:master Nov 13, 2024
51 checks passed
@welcome Welcome
Copy link

welcome bot commented Nov 13, 2024

Congrats on merging your first pull request! 🎉

@bearomorphism bearomorphism deleted the docs/flyte-make-dev-docs branch December 3, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MortalHappiness MortalHappiness MortalHappiness left review comments

@wild-endeavor wild-endeavor wild-endeavor approved these changes

@jasonlai1218 jasonlai1218 jasonlai1218 left review comments

@neverett neverett neverett left review comments

@ppiegaze ppiegaze Awaiting requested review from ppiegaze ppiegaze is a code owner

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bearomorphism @Mecoli1219 @Future-Outlier @neverett @wild-endeavor @jasonlai1218 @MortalHappiness