-
Notifications
You must be signed in to change notification settings - Fork 63
Feat/add read nums #144
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
Feat/add read nums #144
Conversation
Summary of ChangesHello @ChenZiHong-Gavin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a read_nums parameter to limit the number of documents read. While the feature is useful, the current implementation has a significant issue regarding non-deterministic output due to the use of ray.data.Dataset.union() before applying the limit. This can affect reproducibility. Additionally, there is a minor issue of missing input validation for the new parameter. My review comment provides details on both issues and a code suggestion to fix the validation.
| if read_nums is not None: | ||
| combined_ds = combined_ds.limit(read_nums) |
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.
This implementation has a couple of issues:
-
Non-determinism (High Severity): The use of
limit()here can lead to non-deterministic results.combined_dsis created usingray.data.Dataset.union()on line 116, which does not guarantee the order of records between runs. Consequently,limit(read_nums)will yield different subsets of data each time, which is problematic for reproducibility. To improve determinism, you should ensure the file processing order is stable (e.g., by sorting file lists). For true determinism, a more significant refactor to avoidunion()might be necessary. -
Missing Input Validation (Medium Severity): The
read_numsparameter is not validated. A negative value will causelimit()to raise aValueError, but it's better to validate inputs explicitly in this function to provide a clearer error message.
Here is a suggestion to address the input validation:
| if read_nums is not None: | |
| combined_ds = combined_ds.limit(read_nums) | |
| if read_nums is not None: | |
| if read_nums < 0: | |
| raise ValueError("read_nums cannot be negative") | |
| combined_ds = combined_ds.limit(read_nums) |
This pull request introduces a new feature to the read function, enabling users to specify a maximum number of documents to process.