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

[Docs] add practical example to improve data management doc #5844

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

DenChenn
Copy link
Member

@DenChenn DenChenn commented Oct 12, 2024

Tracking issue

Related to #4456

Why are the changes needed?

Based on user feedback, some users understand the basics of data management in Flyte but are unsure how to implement it, so I’ve added examples to help them follow along more easily.

Docs link

https://flyte--5844.org.readthedocs.build/en/5844/user_guide/concepts/main_concepts/data_management.html

Copy link

welcome bot commented Oct 12, 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).

Comment on lines 184 to 185

.. code-block:: python
@task()
def task_remove_column(input_file: FlyteFile, column_name: str) -> FlyteFile:
"""
Copy link
Member

Choose a reason for hiding this comment

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

Not sure is this the right way or should we move to flytesnacks

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

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

Can you add the part stack and heap Ketan mentioned?
He gave an extremely well analogy

@DenChenn
Copy link
Member Author

DenChenn commented Oct 12, 2024

Can you add the part stack and heap Ketan mentioned? He gave an extremely well analogy

Good idea! fixed

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.72%. Comparing base (197ae13) to head (7254acb).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5844      +/-   ##
==========================================
+ Coverage   34.48%   36.72%   +2.23%     
==========================================
  Files        1138     1304     +166     
  Lines      102742   130072   +27330     
==========================================
+ Hits        35434    47764   +12330     
- Misses      63634    78138   +14504     
- Partials     3674     4170     +496     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (+0.21%) ⬆️
unittests-flyteadmin 54.43% <ø> (-1.17%) ⬇️
unittests-flytecopilot 11.73% <ø> (-0.45%) ⬇️
unittests-flytectl 62.40% <ø> (?)
unittests-flyteidl 6.89% <ø> (-0.29%) ⬇️
unittests-flyteplugins 53.62% <ø> (+0.27%) ⬆️
unittests-flytepropeller 42.84% <ø> (+0.82%) ⬆️
unittests-flytestdlib 54.78% <ø> (-0.57%) ⬇️

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.

@DenChenn DenChenn force-pushed the docs/improve-data-management branch from f0f0d69 to 10c34dc Compare October 14, 2024 06:09
@DenChenn DenChenn force-pushed the docs/improve-data-management branch 2 times, most recently from cf42ec7 to 9d96f59 Compare October 16, 2024 04:41
@DenChenn DenChenn requested a review from pingsutw October 16, 2024 04:42
@DenChenn DenChenn force-pushed the docs/improve-data-management branch 3 times, most recently from 30245c1 to c81b534 Compare October 16, 2024 07:35
neverett
neverett previously approved these changes Oct 16, 2024
Copy link
Contributor

@neverett neverett left a comment

Choose a reason for hiding this comment

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

Left a few suggestions, but otherwise LGTM

@DenChenn
Copy link
Member Author

@neverett Thanks for reviewing! done.

@DenChenn DenChenn requested a review from neverett October 17, 2024 04:31
@DenChenn DenChenn force-pushed the docs/improve-data-management branch from fea2977 to 489051f Compare October 17, 2024 11:47
@davidmirror-ops
Copy link
Contributor

@DenChenn can you check the failing docs build test?

@neverett
Copy link
Contributor

Looks like a connection error -- I can rerun the test.

@davidmirror-ops davidmirror-ops enabled auto-merge (squash) October 17, 2024 19:13
@DenChenn
Copy link
Member Author

@neverett Thanks!

Future-Outlier and others added 2 commits October 18, 2024 13:10
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: DenChenn <[email protected]>
auto-merge was automatically disabled October 18, 2024 05:34

Head branch was pushed to by a user without write access


.. note:
Metadata and raw data can be present in entirely separate buckets.
<a href="https://github.com/flyteorg/flyte/blob/6c4f8dbfc6d23a0cd7bf81480856e9ae1dfa1b27/flytepropeller/pkg/controller/config/config.go#L184-L192">View source code on GitHub</a>
Copy link
Member Author

@DenChenn DenChenn Oct 18, 2024

Choose a reason for hiding this comment

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

@Future-Outlier Since .rst doesn't support remote links, I'm using HTML to reference the gitsha link.

@Future-Outlier Future-Outlier merged commit 38af4ec into flyteorg:master Oct 18, 2024
50 checks passed
Copy link

welcome bot commented Oct 18, 2024

Congrats on merging your first pull request! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants