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
docs(contribute): rewrite flyte contribute docs based on 4960 #5260
base: master
Are you sure you want to change the base?
docs(contribute): rewrite flyte contribute docs based on 4960 #5260
Conversation
Signed-off-by: Yu-Ting Hsiung <[email protected]>
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5260 +/- ##
===========================================
+ Coverage 58.60% 76.78% +18.17%
===========================================
Files 568 18 -550
Lines 51121 1516 -49605
===========================================
- Hits 29958 1164 -28794
+ Misses 18748 288 -18460
+ Partials 2415 64 -2351
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
* Install `conda-lock <https://github.com/conda/conda-lock>`__. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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
Related PRs
#4960
Docs link