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

[yaml] add FileIO docs #33185

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[yaml] add FileIO docs #33185

wants to merge 5 commits into from

Conversation

Polber
Copy link
Contributor

@Polber Polber commented Nov 21, 2024

Add docs for all the FileIO YAML Providers. This includes separating out the providers into standalone function calls rather than using a remaning provider referencing th source transform. This allows for YAML-specific docs and possibe future YAML-specific pre/post processing


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@Polber
Copy link
Contributor Author

Polber commented Nov 21, 2024

R: @robertwb

@Polber
Copy link
Contributor Author

Polber commented Nov 21, 2024

R: @damccorm

Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Thanks

@damccorm
Copy link
Contributor

Oh, please fix the precommits

@damccorm
Copy link
Contributor

Looks like precommits are still failing

@Polber Polber force-pushed the jkinard/file-docs branch 3 times, most recently from 629390a to 50b4cef Compare December 4, 2024 15:45
Signed-off-by: Jeffrey Kinard <[email protected]>
Signed-off-by: Jeffrey Kinard <[email protected]>
Signed-off-by: Jeffrey Kinard <[email protected]>
@Polber Polber force-pushed the jkinard/file-docs branch from 50b4cef to 2eac016 Compare December 4, 2024 16:30
Signed-off-by: Jeffrey Kinard <[email protected]>
@Polber Polber force-pushed the jkinard/file-docs branch from 2eac016 to c90624e Compare December 4, 2024 16:31
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 60.60606% with 13 lines in your changes missing coverage. Please review.

Project coverage is 57.42%. Comparing base (a9a82a1) to head (c90624e).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
sdks/python/apache_beam/yaml/generate_yaml_docs.py 0.00% 7 Missing ⚠️
sdks/python/apache_beam/yaml/yaml_io.py 76.92% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #33185   +/-   ##
=========================================
  Coverage     57.41%   57.42%           
  Complexity     1474     1474           
=========================================
  Files           970      970           
  Lines        154498   154526   +28     
  Branches       1076     1076           
=========================================
+ Hits          88708    88732   +24     
- Misses        63586    63590    +4     
  Partials       2204     2204           
Flag Coverage Δ
python 81.40% <60.60%> (+<0.01%) ⬆️

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.

Copy link
Contributor

@robertwb robertwb left a comment

Choose a reason for hiding this comment

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

I'm not sure we want to go this direction; re-using the docs from the original transforms rather than duplicating them is a feature, not a bug. Certainly they could be improved (which would improve both).

@@ -86,10 +86,16 @@ def _fake_value(name, beam_type):
raise ValueError(f"Unrecognized type_info: {type_info!r}")


EXCLUDE_ARGS = ['args', 'kwargs']
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't always filter these out, e.g. https://beam.apache.org/releases/yamldoc/current/#pytransform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah.. right.. let me rethink

sdks/python/apache_beam/yaml/generate_yaml_docs.py Outdated Show resolved Hide resolved
<https://pandas.pydata.org/docs/reference/api/pandas.read_json.html>

Args:
path (str): The file path to read from as a local file path or a
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we support more than local files and gcs; the set of filesystems is not closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I also found that limiting, but I copied that description from another docstring, e.g.

file_pattern (str): The file path to read from as a local file path or a
GCS ``gs://`` path. The path can contain glob characters
(``*``, ``?``, and ``[...]`` sets).

Signed-off-by: Jeffrey Kinard <[email protected]>
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.

3 participants