Spec URL: https://qulogic.fedorapeople.org//python-distributed.spec SRPM URL: https://qulogic.fedorapeople.org//python-distributed-1.26.0-1.fc29.src.rpm Description: Dask.distributed is a lightweight library for distributed computing in Python. It extends both the concurrent.futures and dask APIs to moderate sized clusters.
Still trying to work out a few issues with tests.
CC me
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.
Yep, still trying to figure out the test failures though, unfortunately.
Nearly working now; will post later today.
This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it.
Reviving this, we really need it now, due to this ridiculous dep cluster: dask -> dask-expr -> distributed -> dask -> distributed (yes, really)
Updated to latest version, though skipping a bunch of things for now. Spec URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed.spec SRPM URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed-2024.6.0-1.fc41.src.rpm
Copr build: https://copr.fedorainfracloud.org/coprs/build/7627011 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-1686307-python-distributed/fedora-rawhide-x86_64/07627011-python-distributed/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
Now down to 2 skips, with Adam's patches, and possibly one of those is a CPython issue. Spec URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed.spec SRPM URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed-2024.6.0-1.fc41.src.rpm
Created attachment 2037823 [details] The .spec file difference from Copr build 7627011 to 7629263
Copr build: https://copr.fedorainfracloud.org/coprs/build/7629263 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-1686307-python-distributed/fedora-rawhide-x86_64/07629263-python-distributed/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
Since there hasn't been a Rawhide compose, that Copr build couldn't find the new versioneer. Here's a koji scratch build instead: https://koji.fedoraproject.org/koji/taskinfo?taskID=119327383 It's still running though as some things appear to be timing out for some reason.
Looks like we're getting https://github.com/dask/distributed/issues/2554 and https://github.com/dask/distributed/issues/2552, but for some reason only on koji, not a local mock.
so, status on remaining fails... * the 0.0.0.0 vs. 127.0.0.1 stuff (#2554) should be fixed by https://github.com/dask/distributed/pull/8712 * the deeply_nested_structures thing seems to be just a test making unsafe assumptions about when and how things will fail - https://github.com/dask/distributed/issues/8700 . we could possibly write a better test, I've floated an idea along those lines in the issue, but I think it's fine to disable it for now * the test_upload_file_zip thing seems to be due to a recent change in cpython zipimport: https://github.com/python/cpython/pull/103208 . I found a way to fix it in cpython: https://github.com/python/cpython/pull/103208#issuecomment-2181682479 . gonna see if we get feedback from upstream to decide how to proceed there * test_git_revision should be fixed by https://github.com/dask/distributed/pull/8709 , I think I saw a version of the spec with that fixed? overall I think we're good enough to ship this, honestly. Will take a step back and give the spec a general review later or tomorrow.
Now updated to latest version: Spec URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed.spec SRPM URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed-2024.6.2-1.fc41.src.rpm
Created attachment 2037896 [details] The .spec file difference from Copr build 7629263 to 7639759
Copr build: https://copr.fedorainfracloud.org/coprs/build/7639759 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-1686307-python-distributed/fedora-rawhide-x86_64/07639759-python-distributed/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
A koji scratch build instead: https://koji.fedoraproject.org/koji/taskinfo?taskID=119373991
Looks like a couple more test blips in the scratch build :/ two in x86_64, one in ppc64le. Not the same ones. They kinda look like timing issues to me, if anything, they don't leap out as being Python 3.13-related or anything. Might be a case where we should just throw builds until one sticks, or disable more tests :/ Doing a review on this...rpmlint outputs: [adamw@xps13a tmp]$ rpmlint python* =================================================================================== rpmlint session starts =================================================================================== rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 4 python-distributed.src: E: spelling-error ('Dask', '%description -l en_US Dask -> Ask, Task, Cask') python-distributed.src: E: spelling-error ('dask', '%description -l en_US dask -> ask, desk, disk') python3-distributed.aarch64: E: spelling-error ('Dask', '%description -l en_US Dask -> Ask, Task, Cask') python3-distributed.aarch64: E: spelling-error ('dask', '%description -l en_US dask -> ask, desk, disk') python3-distributed.s390x: E: spelling-error ('Dask', '%description -l en_US Dask -> Ask, Task, Cask') python3-distributed.s390x: E: spelling-error ('dask', '%description -l en_US dask -> ask, desk, disk') python-distributed.spec: W: patch-not-applied Patch0: 0009-get_ip-handle-getting-0.0.0.0-2554.patch python-distributed.spec: W: patch-not-applied Patch0: 0009-get_ip-handle-getting-0.0.0.0-2554.patch python3-distributed.aarch64: W: no-manual-page-for-binary dask-scheduler python3-distributed.aarch64: W: no-manual-page-for-binary dask-ssh python3-distributed.aarch64: W: no-manual-page-for-binary dask-worker python3-distributed.s390x: W: no-manual-page-for-binary dask-scheduler python3-distributed.s390x: W: no-manual-page-for-binary dask-ssh python3-distributed.s390x: W: no-manual-page-for-binary dask-worker python3-distributed.aarch64: E: no-binary python3-distributed.s390x: E: no-binary ============================================= 3 packages and 1 specfiles checked; 8 errors, 8 warnings, 39 filtered, 8 badness; has taken 1.4 s ============================================== The spelling-error is obviously false, the patch-not-applied also seems to be some kind of blip (the patch is listed as Patch: not Patch0:, and the spec uses %forgeautosetup -p1 so it should be applied). no-manual-page-for-binary is legit enough, I guess, if there is one we should package it, if not we should request it upstream. no-binary is because the python3-distributed package is arched. There is a comment in the spec: # We have an arched package to detect arch-dependent issues in dependencies, # but all of the installable RPMs are noarch and there is no compiled code. but that seems to not be accurate, it seems to have been copied from python-dask.spec, but the python3 subpackage doesn't have `BuildArch: noarch` as it does in python-dask.spec. (Though it's actually a lie for python-dask now too, since the "+foo" subpackages are arched). Probably needs fixing. "MUST: The package must meet the Packaging Guidelines" - well, it seems to violate "All patches should have an upstream bug link or comment". The only comment before the first three patches is "Fedora specific?" - that doesn't explain what they do, why they're Fedora specific (i.e. not upstreamable), and it's not even clear that comment applies to all three patches. 0005-Loosen-up-some-dependencies.patch has no comment. The comment for 0008-Avoid-using-sys.prefix-in-CLI-test.patch does not list an upstream reference or clearly explain why it's downstream-only. Personally I would also include short descriptions of each patch as well as PR links, but that's more a personal preference. "MUST: The License field in the package spec file must match the actual license. See Licensing Guidelines: Valid License Short Names" - I don't think this is fully met. First off, new spec files must use SPDX license identifiers, and "BSD" isn't one, according to https://spdx.org/licenses/ - you need to use a more specific identifier for exactly what flavor of BSD license this is. Second, there seem to be files in the package under licenses other than BSD: distributed/compatibility.py and distributed/threadpoolexecutor.py contain code under the Python license, and distributed/http/static/js/anime.min.js and distributed/http/static/js/reconnecting-websocket.min.js are under the MIT license. These licenses must also be listed, in SPDX style. "SHOULD: your package should contain man pages for binaries/scripts. If it doesn’t, work with upstream to add them where they make sense. See Packaging Guidelines: Manpages" - see above. I think those are all the outstanding issues I can see. Marking NEEDINFO for those to be worked on.
Miro: do you have any thoughts on the arch stuff? It feels a bit weird to me - the way it is in python-dask, which has been kinda-inherited here. I feel like these should just be noarch. I don't really buy "# We have an arched package to detect arch-dependent issues in dependencies"...
Hmm, okay, so it seems like it was Ben Beasley's idea, and the idea was to force dask to build on all arches so the tests would run on all arches and catch any problems, but the installable subpackages would all be noarch. However, it never worked out this way, because he didn't think about the %pyproject_extras_subpkg subpackages. Evaluating that macro doesn't give you "BuildArch: noarch": [adamw@xps13a python-dask (rawhide %)]$ rpm --eval "%pyproject_extras_subpkg -n python3-foo bar" %package -n python3-foo+bar Summary: Metapackage for python3-foo: bar extras Requires: python3-foo = %{version}-%{release} %description -n python3-foo+bar This is a metapackage bringing in bar extras requires for python3-foo. It makes sure the dependencies are installed. %files -n python3-foo+bar -f /home/adamw/rpmbuild/BUILD/%{name}-%{version}-%{release}.x86_64-pyproject-ghost-distinfo so if the main package is arched, any subpackage produced with the macro will also be arched. That's a problem for dask, I guess, I'll file a bug there. python-distributed doesn't have any extras subpackages yet, so we can achieve Ben's intent by just marking the python3-distributed subpackage as noarch, if we do want to force builds across all arches.
(In reply to Adam Williamson from comment #24) > Hmm, okay, so it seems like it was Ben Beasley's idea, and the idea was to > force dask to build on all arches so the tests would run on all arches and > catch any problems, but the installable subpackages would all be noarch. > However, it never worked out this way, because he didn't think about the > %pyproject_extras_subpkg subpackages. I think the general idea with that approach was/is that it builds on all arches indeed. Because with `BuildArch: noarch` you don't know which builder will be chosen and it is annoying seeing you scratch build succeed and the real build fail because it is on a different arch. But I let Ben speak for himself (cc'ed).
(In reply to Sandro from comment #25) > (In reply to Adam Williamson from comment #24) > > Hmm, okay, so it seems like it was Ben Beasley's idea, and the idea was to > > force dask to build on all arches so the tests would run on all arches and > > catch any problems, but the installable subpackages would all be noarch. > > However, it never worked out this way, because he didn't think about the > > %pyproject_extras_subpkg subpackages. > > I think the general idea with that approach was/is that it builds on all > arches indeed. Because with `BuildArch: noarch` you don't know which builder > will be chosen and it is annoying seeing you scratch build succeed and the > real build fail because it is on a different arch. But I let Ben speak for > himself (cc'ed). Yes, that’s the motivation. In neuro-sig, we do this in a lot of numerical and file-format library packages because arch-dependent test failures are so very common. Often we add it to previously-noarch packages when we find and fix an arch-dependent issue. To be honest, I *knew* that the extras metapackages would be arched, but I didn’t feel that it *mattered*, because metapackages don’t ship any files so the wasted space on mirrors by having a cooy for each architecture is negligible. For python-distrubuted, there’s some risk of arch-dependent issues in the shuffling or serialization/deserialization code, but I also think it would be justifiable to start out with it noarch and then revisit that it if it develops one or more arch-dependent packages in practice. More discussion in bug 2293727.
(In reply to Adam Williamson from comment #22) > Looks like a couple more test blips in the scratch build :/ two in x86_64, > one in ppc64le. Not the same ones. They kinda look like timing issues to me, > if anything, they don't leap out as being Python 3.13-related or anything. > Might be a case where we should just throw builds until one sticks, or > disable more tests :/ > I fixed a couple, but the others seemed random. > no-binary is > because the python3-distributed package is arched. There is a comment in the > spec: > > # We have an arched package to detect arch-dependent issues in dependencies, > # but all of the installable RPMs are noarch and there is no compiled code. > > but that seems to not be accurate, it seems to have been copied from > python-dask.spec, but the python3 subpackage doesn't have `BuildArch: > noarch` as it does in python-dask.spec. (Though it's actually a lie for > python-dask now too, since the "+foo" subpackages are arched). Probably > needs fixing. This is now fixed. > "MUST: The package must meet the Packaging Guidelines" - well, it seems to > violate "All patches should have an upstream bug link or comment". The only > comment before the first three patches is "Fedora specific?" - that doesn't > explain what they do, why they're Fedora specific (i.e. not upstreamable), > and it's not even clear that comment applies to all three patches. > 0005-Loosen-up-some-dependencies.patch has no comment. The comment for > 0008-Avoid-using-sys.prefix-in-CLI-test.patch does not list an upstream > reference or clearly explain why it's downstream-only. Personally I would > also include short descriptions of each patch as well as PR links, but > that's more a personal preference. > I re-ordered these so all the Fedora ones are together. I consider the patch names to be descriptive enough, and since they were cherry-picked and `git format-patch`d into a series, they contain the information from the original commits, so I don't see the need to re-describe them in the spec file. > "MUST: The License field in the package spec file must match the actual > license. See Licensing Guidelines: Valid License Short Names" - I don't > think this is fully met. First off, new spec files must use SPDX license > identifiers, and "BSD" isn't one, according to https://spdx.org/licenses/ - > you need to use a more specific identifier for exactly what flavor of BSD > license this is. Second, there seem to be files in the package under > licenses other than BSD: distributed/compatibility.py and > distributed/threadpoolexecutor.py contain code under the Python license, and > distributed/http/static/js/anime.min.js and > distributed/http/static/js/reconnecting-websocket.min.js are under the MIT > license. These licenses must also be listed, in SPDX style. Yes, this review is older than SPDX... It's now correct and includes the licence breakdown in a comment. > "SHOULD: your package should contain man pages for binaries/scripts. If it > doesn’t, work with upstream to add them where they make sense. See Packaging > Guidelines: Manpages" - see above. > I doubt upstream would write them, but I suppose I could ask. > I think those are all the outstanding issues I can see. Marking NEEDINFO for > those to be worked on. Spec URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed.spec SRPM URL: https://qulogic.fedorapeople.org/reviews/python-distributed/python-distributed-2024.6.2-1.fc41.src.rpm
Created attachment 2037986 [details] The .spec file difference from Copr build 7639759 to 7650143
Copr build: https://copr.fedorainfracloud.org/coprs/build/7650143 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-1686307-python-distributed/fedora-rawhide-x86_64/07650143-python-distributed/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
It looks like the COPR build failed because test_client_worker is supposed to take less than 3 seconds and it took 3.01 seconds. :P
Cool, that looks a lot better. As a nitpick, shouldn't 0008-Avoid-using-sys.prefix-in-CLI-test.patch be up with the "Fedora-specific" patches? It doesn't really look upstreamable. Or if you think upstream might take it, it should have a PR link :) But regardless, I'd say this is now approved. For the one test that failed on the COPR build we could patch that time limit to 4 seconds, or just rely on spamming builds, I guess.
I also accidentally wrote Apache-2.0.1 (like Python-2.0.1) instead of Apache-2.0 in the license once, so I'll fix those on import.
Thank you for the review, Adam
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-distributed
I think this is done: https://bodhi.fedoraproject.org/updates/FEDORA-2024-5e71348961