cleverdatasets/dataset-uploader
cleverdatasets/dataset-uploader
0
0
Fork 0
Code Issues 17 Pull requests 6 Projects Releases Packages Wiki Activity Actions

16-error-messages-and-logging #17

Open
brent.edwards wants to merge 11 commits from 16-error-messages-and-logging into master
pull from: 16-error-messages-and-logging
merge into: cleverdatasets:master
Conversation 1 Commits 11 Files changed 8 +832 -370
brent.edwards commented 2025-11-21 01:39:06 +00:00
Member
Copy link

I used qwen3-coder to suggest additions for error messages. They seemed good. Please read through them and make sure that I didn't approve things that I should not have.

I used qwen3-coder to suggest additions for error messages. They seemed good. Please read through them and make sure that *I* didn't approve things that I should not have.
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) <aider@aider.chat>
khird reviewed 2025-12-05 14:59:33 +00:00
khird left a comment
First-time contributor
Copy link

Mostly suggestions, but the big commented block in the middle is more concerning.

Mostly suggestions, but the big commented block in the middle is more concerning.
scripts/convert_nell995_to_hf.py
@ -92,1 +103,4 @@
console.print(f" - {path}")
# List what files are actually there
try:
all_files = list(input_dir.rglob("*"))
khird commented 2025-12-05 13:54:00 +00:00
First-time contributor
Copy link

Would you rather use a globstar here? You're searching in nested directories, so just seeing the contents of the top-level dir mightn't be as useful as giving the user the whole tree. I can imagine that if the program runs in a context where it doesn't have traverse permissions on one of the intermediate dirs, it'd be very helpful to the user to see immediately that the file tree accessible to the program is different from what he can see himself.

Would you rather use a globstar here? You're searching in nested directories, so just seeing the contents of the top-level dir mightn't be as useful as giving the user the whole tree. I can imagine that if the program runs in a context where it doesn't have traverse permissions on one of the intermediate dirs, it'd be very helpful to the user to see immediately that the file tree accessible to the program is different from what he can see himself.
scripts/convert_rdf_to_hf_dataset.py
@ -719,0 +741,4 @@
ProgressFileReader(f, update_progress, file_size_bytes) as pf,
):
file_content = pf.read()
except (gzip.BadGzipFile, bz2.BadGzipFile) as e:
khird commented 2025-12-05 14:02:29 +00:00
First-time contributor
Copy link

I don't see BadGzipFile in bz2. From source here:

__all__ = ["BZ2File", "BZ2Compressor", "BZ2Decompressor",
           "open", "compress", "decompress"]
I don't see `BadGzipFile` in `bz2`. From source [here](https://github.com/python/cpython/blob/3.14/Lib/bz2.py): ``` __all__ = ["BZ2File", "BZ2Compressor", "BZ2Decompressor", "open", "compress", "decompress"] ```
scripts/convert_rdf_to_hf_dataset_streaming_parallel.py
@ -618,3 +695,1 @@
total_triples = 0
chunk_count = 0
parquet_files = []
total_triples = 0 # DELETE THIS LINE!
khird commented 2025-12-05 14:14:07 +00:00
First-time contributor
Copy link

This looks like WIP code; did you forget to change something before filing the PR? I see a block of ninety-odd lines of commented-out code, minus three lines that were left active, and one of the three has an all-caps comment to DELETE THIS LINE!.

This looks like WIP code; did you forget to change something before filing the PR? I see a block of ninety-odd lines of commented-out code, minus three lines that were left active, and one of the three has an all-caps comment to `DELETE THIS LINE!`.
khird commented 2025-12-05 14:18:18 +00:00
First-time contributor
Copy link

Yeah, later on I see that you're still trying to operate on the Parquet files despite having commented out the place where they're generated. If this is what's supposed to be happening, could you please make your intent clearer?

Yeah, later on I see that you're still trying to operate on the Parquet files despite having commented out the place where they're generated. If this is what's supposed to be happening, could you please make your intent clearer?
scripts/rdf_dataset_downloader.py
@ -221,0 +252,4 @@
# Check if file size is reasonable compared to expected size
if dataset_info.compressed_size_gb:
expected_bytes = dataset_info.compressed_size_gb * 1024**3
if file_size < expected_bytes * 0.1: # Less than 10% of expected size
khird commented 2025-12-05 14:31:06 +00:00
First-time contributor
Copy link

Please double-check this logic; quite possibly it's correct but I'm unsure. If compressed_size actually refers to the compressed size (i.e. the size of the .gz/.bz2 file) then I would expect the file_size to be (very roughly) a factor of ten larger, not smaller, than the compressed size. This comparison looks reasonably correct if you mean the uncompressed size - if that's the case, I think it'd be clearer to say "uncompressed"/"decompressed" in the comment rather than "expected".

Please double-check this logic; quite possibly it's correct but I'm unsure. If `compressed_size` actually refers to the compressed size (i.e. the size of the `.gz`/`.bz2` file) then I would expect the `file_size` to be (very roughly) a factor of ten *larger*, not smaller, than the compressed size. This comparison looks reasonably correct if you mean the uncompressed size - if that's the case, I think it'd be clearer to say "uncompressed"/"decompressed" in the comment rather than "expected".
scripts/upload_all_datasets.py
@ -574,9 +574,14 @@ def process_dataset(
Returns:
True if successful, False otherwise
khird commented 2025-12-05 14:39:07 +00:00
First-time contributor
Copy link

This can also return None; both the docstring and the type annotation claim otherwise.

This can also return None; both the docstring and the type annotation claim otherwise.
scripts/upload_all_datasets.py
@ -889,2 +925,3 @@
# Filter out README files if there are other options
non_readme_files = [f for f in rdf_files if "readme" not in f.name.lower()]
non_readme_files = [f for f in rdf_files if "readme" not in f.name.lower()
and f.name.endswith(".bz2")]
khird commented 2025-12-05 14:49:21 +00:00
First-time contributor
Copy link

This looks incorrect. Suppose you have rdf_files == ["README", "foo.rdf"] this change will make non_readme_files empty and then you'll pick "README" out of rdf_files in a few lines.

This looks incorrect. Suppose you have `rdf_files == ["README", "foo.rdf"]` this change will make `non_readme_files` empty and then you'll pick `"README"` out of `rdf_files` in a few lines.
scripts/upload_all_datasets.py
@ -1008,3 +1003,1 @@
rdf_file = rdf_files[0]
# Test again
if rdf_file.endswith(".bz2"):
khird commented 2025-12-05 14:54:26 +00:00
First-time contributor
Copy link

In what is now line 934, you checked this as if str(rdf_file).endswith(".bz2"):. Is the cast to string necessary here too, or do we know a priori that it's something that supports .endswith()?

In what is now line 934, you checked this as `if str(rdf_file).endswith(".bz2"):`. Is the cast to string necessary here too, or do we know a priori that it's something that supports `.endswith()`?
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 16-error-messages-and-logging:16-error-messages-and-logging
git switch 16-error-messages-and-logging

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff 16-error-messages-and-logging
git switch 16-error-messages-and-logging
git rebase master
git switch master
git merge --ff-only 16-error-messages-and-logging
git switch 16-error-messages-and-logging
git rebase master
git switch master
git merge --no-ff 16-error-messages-and-logging
git switch master
git merge --squash 16-error-messages-and-logging
git switch master
git merge --ff-only 16-error-messages-and-logging
git switch master
git merge 16-error-messages-and-logging
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
  
Blocked

A ticket in a blocked state and unable to complete until some other task is completed first.

  
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.

  
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.

  
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.

  
Needs feedback

There are questions in the ticket that can not be completed until the project owner provides clarity.

  
Points
1
1 man-hours worth of work for an expert with no learning curve.

  
Points
13
13 man-hours worth of work for an expert with no learning curve.

  
Points
2
2 man-hours worth of work for an expert with no learning curve.

  
Points
21
21 man-hours worth of work for an expert with no learning curve.

  
Points
3
3 man-hours worth of work for an expert with no learning curve.

  
Points
34
34 man-hours worth of work for an expert with no learning curve.

  
Points
5
5 man-hours worth of work for an expert with no learning curve.

  
Points
55
55 man-hours worth of work for an expert with no learning curve.

  
Points
8
8 man-hours worth of work for an expert with no learning curve.

  
Points
88
88 man-hours worth of work for an expert with no learning curve.

  
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Signed-off: Owner

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Scrum Master

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Tech Lead

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Spike

A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  
State
Duplicate
A ticket that represents the same content as an existing ticket.

  
State
In Progress
A ticket that is actively being developed.

  
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  
Type
Bug
Something that doesnt work as intended.

  
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  
Type
Documentation
An error or improvement needed in the documentation.

  
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  
Type
Feature
Some new functionality not present.

  
Type
Legendary
A type of Epic which will contain other Epics.

  
Type
Support
Someone needs help using the project.

  
Type
Task
A generic task that doesnt fit into the other type categories.

  
Type
Testing
Work exclusively focusing on fixing or expanding testing.

No labels
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Support
Type
Task
Type
Testing
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleverdatasets/dataset-uploader!17
No description provided.