On Mon Apr 20, 2026 at 2:29 PM CEST, Jeroen Ploemen wrote: > did a review of python-papermill, up for sponsorship in the Python > Team:
Hi Jeroen, Thanks for this detailed review. You may have seen that Bastian has uploaded python-papermill in parallel. I will implement your comments and make them part of a next version. Some specifics: > * control: the restriction on pyarrow only deals with i386, > presumably because the salsa CI doesn't try any archs other than > amd64 and i386. The Debian CI on the other hand runs on more archs; > does the current setup survive that, or is this package going to > run into trouble elsewhere as well? If so, you might want to switch > the current [!i386] to a list of all archs where pyarrow is > available. I should have looked at the apache-arrow debian/control file. [1] apache-arrow restricts to 64-bit LE and that means I should exclude not only i386. > A similar question applies to the patch that skips the pyarrow > tests on i386. That could be generalised to check for the > availability of pyarrow, rather than looking for any specific > architecture(s). Yes, this is the consequence. > * patches: the indentation issue that > fix_indent_error_in_test_iorw.py.patch deals with isn't present in > the upstream code, but introduced via > remove-relative-paths-and-entrypoints.diff. Please drop the former > and instead fix the latter. Ouch. I should have checked. Will do and will report this upstream. > > * patches: what's the reasoning behind the location change in > fix_test_s3_locationconstraint.patch? I got an IllegalLocationConstraintException from moto. However, when trying to retrace my steps and find the stackoverflow or other source where I found the suggestion to change the region to fix that error I bumped into a post [2] explaining that moto / boto3 can take info from outside the project context. So, my own config might be contaminating this test. Though I don't see right away how that would the correlate to the builds in sbuild or salsa CI. I will double check this. Thanks again! Pieter [1]: https://salsa.debian.org/science-team/arrow/-/blob/master/debian/control?ref_type=heads#L12 [2]: https://til.codeinthehole.com/posts/python-tests-using-moto-should-be-explicit-about-aws-regions/
signature.asc
Description: PGP signature
