Bug 2253031 - Review Request: python-torchdata - A PyTorch module for data loading
Summary: Review Request: python-torchdata - A PyTorch module for data loading
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Tim Flink
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/pytorch/data
Whiteboard:
Depends On:
Blocks: ML-SIG
TreeView+ depends on / blocked
 
Reported: 2023-12-05 17:53 UTC by Tom Rix
Modified: 2023-12-16 16:46 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-12-16 16:46:12 UTC
Type: ---
Embargoed:
tflink: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 6726866 to 6735570 (1020 bytes, patch)
2023-12-08 23:39 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 6735570 to 6758491 (996 bytes, patch)
2023-12-15 02:06 UTC, Fedora Review Service
no flags Details | Diff

Description Tom Rix 2023-12-05 17:53:06 UTC
Spec URL: https://trix.fedorapeople.org/python-torchdata.spec
SRPM URL: https://trix.fedorapeople.org/python-torchdata-0.7.0-1.fc40.src.rpm

torchdata is a library of common modular data loading primitives for                                                                                         
easily constructing data pipelines. 

Reproducible: Always

Comment 1 Fedora Review Service 2023-12-05 17:59:59 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6726866
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2253031-python-torchdata/fedora-rawhide-x86_64/06726866-python-torchdata/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 2 Tim Flink 2023-12-06 21:55:22 UTC
Looks good overall but two minor things:

%{python3_sitelib}/torchdata is listed twice - once by the script that you have detecting the python modules and once by direct declaration. only one is needed

is there a reason that the tests aren't being run in %check? if so, can you add a comment to that effect in the specfile?

Comment 3 Tom Rix 2023-12-08 12:44:41 UTC
Spec URL: https://trix.fedorapeople.org/python-torchdata.spec
SRPM URL: https://trix.fedorapeople.org/python-torchdata-0.7.0-2.fc40.src.rpm

The listed twice is a warning, not an error, is caused by the %dir declarations.

I have commented on why no check, it depends on expecttest
https://github.com/ezyang/expecttest
That looks like it is a low value to package.

However if you will take the review of expecttest, I will add it.

Comment 4 Fedora Review Service 2023-12-08 23:39:17 UTC
Created attachment 2003352 [details]
The .spec file difference from Copr build 6726866 to 6735570

Comment 5 Fedora Review Service 2023-12-08 23:39:21 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6735570
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2253031-python-torchdata/fedora-rawhide-x86_64/06735570-python-torchdata/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 6 Tim Flink 2023-12-14 18:02:02 UTC
(In reply to Tom Rix from comment #3)
> Spec URL: https://trix.fedorapeople.org/python-torchdata.spec
> SRPM URL: https://trix.fedorapeople.org/python-torchdata-0.7.0-2.fc40.src.rpm
> 
> The listed twice is a warning, not an error, is caused by the %dir
> declarations.
> 
> I have commented on why no check, it depends on expecttest
> https://github.com/ezyang/expecttest
> That looks like it is a low value to package.
> 
> However if you will take the review of expecttest, I will add it.

Looking at expecttest, I agree that it's probably not worth packaging. The concept is interesting but according to libraries.io [1], it's only used by 2 packages on pypi.

For the listed twice thing, I get that it's a warning but is there a reason for it to be listed twice? I would have thought that the sitelib dir would be found by echo+find+sed or by listing the sitelib dir and egg file manually. Why do it twice?

[1] https://libraries.io/pypi/expecttest

Comment 7 Tom Rix 2023-12-15 02:01:36 UTC
Spec URL: https://trix.fedorapeople.org/python-torchdata.spec
SRPM URL: https://trix.fedorapeople.org/python-torchdata-0.7.0-3.fc40.src.rpm

I removed the generated %dir's, this clears the warning

Comment 8 Fedora Review Service 2023-12-15 02:06:55 UTC
Created attachment 2004364 [details]
The .spec file difference from Copr build 6735570 to 6758491

Comment 9 Fedora Review Service 2023-12-15 02:06:57 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6758491
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2253031-python-torchdata/fedora-rawhide-x86_64/06758491-python-torchdata/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 10 Tim Flink 2023-12-15 16:33:05 UTC
LGTM

Comment 11 Fedora Admin user for bugzilla script actions 2023-12-16 00:03:22 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-torchdata


Note You need to log in before you can comment on or make changes to this bug.